From 706137d6330c1611066dd74f3613338a161aac93 Mon Sep 17 00:00:00 2001 From: Devin Avery Date: Thu, 29 May 2014 16:42:03 -0400 Subject: [PATCH] Bug 1100 - Invoking an RPC with no input should not throw 500 when expected Patch Set 3: *Modified the Json/Xml readers to not throw an except if the content is empty *Moved the validation of non empty object into restconf where the knowledge of what is expected exists. Change-Id: I4e689770d1a6843e99ce498bd12cecc5e1817eaa Signed-off-by: Devin Avery --- .../controller/sal/rest/impl/JsonReader.java | 6 ++ .../controller/sal/rest/impl/XmlReader.java | 39 +++++++++- .../sal/restconf/impl/RestconfImpl.java | 71 +++++++++++++------ .../json/to/cnsn/test/JsonToCnSnTest.java | 11 +++ .../impl/test/InvokeRpcMethodTest.java | 2 +- .../impl/test/RestPostOperationTest.java | 4 ++ .../impl/test/RestPutOperationTest.java | 12 ++++ .../impl/xml/to/cnsn/test/XmlToCnSnTest.java | 27 +++++++ 8 files changed, 148 insertions(+), 24 deletions(-) diff --git a/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/JsonReader.java b/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/JsonReader.java index 4d9958ee6b..cdea81f2b7 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/JsonReader.java +++ b/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/JsonReader.java @@ -31,6 +31,12 @@ class JsonReader { JsonParser parser = new JsonParser(); JsonElement rootElement = parser.parse(new InputStreamReader(entityStream)); + if( rootElement.isJsonNull() ) + { + //no content, so return null to indicate no input + return null; + } + if (!rootElement.isJsonObject()) { throw new UnsupportedFormatException("Root element of Json has to be Object"); } diff --git a/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/XmlReader.java b/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/XmlReader.java index a75f6b4a85..2965ae7209 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/XmlReader.java +++ b/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/rest/impl/XmlReader.java @@ -9,6 +9,8 @@ package org.opendaylight.controller.sal.rest.impl; import static com.google.common.base.Preconditions.checkArgument; +import java.io.BufferedInputStream; +import java.io.IOException; import java.io.InputStream; import java.net.URI; import java.util.Stack; @@ -32,7 +34,17 @@ public class XmlReader { private final static XMLInputFactory xmlInputFactory = XMLInputFactory.newInstance(); private XMLEventReader eventReader; - public CompositeNodeWrapper read(InputStream entityStream) throws XMLStreamException, UnsupportedFormatException { + public CompositeNodeWrapper read(InputStream entityStream) throws XMLStreamException, + UnsupportedFormatException, + IOException { + //Get an XML stream which can be marked, and reset, so we can check and see if there is + //any content being provided. + entityStream = getMarkableStream(entityStream); + + if( isInputStreamEmpty( entityStream ) ) { + return null; + } + eventReader = xmlInputFactory.createXMLEventReader(entityStream); if (eventReader.hasNext()) { @@ -91,6 +103,31 @@ public class XmlReader { return root; } + /** + * If the input stream is not markable, then it wraps the input stream with a buffered stream, + * which is mark able. That way we can check if the stream is empty safely. + * @param entityStream + * @return + */ + private InputStream getMarkableStream(InputStream entityStream) { + if( !entityStream.markSupported() ) + { + entityStream = new BufferedInputStream( entityStream ); + } + return entityStream; + } + + private boolean isInputStreamEmpty(InputStream entityStream) + throws IOException { + boolean isEmpty = false; + entityStream.mark( 1 ); + if( entityStream.read() == -1 ){ + isEmpty = true; + } + entityStream.reset(); + return isEmpty; + } + private boolean isSimpleNodeEvent(final XMLEvent event) throws XMLStreamException { checkArgument(event != null, "XML Event cannot be NULL!"); if (event.isStartElement()) { diff --git a/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/restconf/impl/RestconfImpl.java b/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/restconf/impl/RestconfImpl.java index ad682bc829..c0ce90e15d 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/restconf/impl/RestconfImpl.java +++ b/opendaylight/md-sal/sal-rest-connector/src/main/java/org/opendaylight/controller/sal/restconf/impl/RestconfImpl.java @@ -8,13 +8,6 @@ */ package org.opendaylight.controller.sal.restconf.impl; -import com.google.common.base.Objects; -import com.google.common.base.Preconditions; -import com.google.common.base.Splitter; -import com.google.common.base.Strings; -import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; - import java.net.URI; import java.text.ParseException; import java.text.SimpleDateFormat; @@ -38,21 +31,11 @@ import org.opendaylight.controller.md.sal.common.api.TransactionStatus; import org.opendaylight.controller.sal.core.api.mount.MountInstance; import org.opendaylight.controller.sal.rest.api.Draft02; import org.opendaylight.controller.sal.rest.api.RestconfService; +import org.opendaylight.controller.sal.restconf.impl.RestconfError.ErrorTag; +import org.opendaylight.controller.sal.restconf.impl.RestconfError.ErrorType; import org.opendaylight.controller.sal.restconf.rpc.impl.BrokerRpcExecutor; import org.opendaylight.controller.sal.restconf.rpc.impl.MountPointRpcExecutor; import org.opendaylight.controller.sal.restconf.rpc.impl.RpcExecutor; -import org.opendaylight.controller.sal.restconf.impl.BrokerFacade; -import org.opendaylight.controller.sal.restconf.impl.CompositeNodeWrapper; -import org.opendaylight.controller.sal.restconf.impl.ControllerContext; -import org.opendaylight.controller.sal.restconf.impl.EmptyNodeWrapper; -import org.opendaylight.controller.sal.restconf.impl.IdentityValuesDTO; -import org.opendaylight.controller.sal.restconf.impl.InstanceIdWithSchemaNode; -import org.opendaylight.controller.sal.restconf.impl.NodeWrapper; -import org.opendaylight.controller.sal.restconf.impl.RestCodec; -import org.opendaylight.controller.sal.restconf.impl.SimpleNodeWrapper; -import org.opendaylight.controller.sal.restconf.impl.StructuredData; -import org.opendaylight.controller.sal.restconf.impl.RestconfError.ErrorTag; -import org.opendaylight.controller.sal.restconf.impl.RestconfError.ErrorType; import org.opendaylight.controller.sal.streams.listeners.ListenerAdapter; import org.opendaylight.controller.sal.streams.listeners.Notificator; import org.opendaylight.controller.sal.streams.websockets.WebSocketServer; @@ -84,6 +67,13 @@ import org.opendaylight.yangtools.yang.model.util.EmptyType; import org.opendaylight.yangtools.yang.parser.builder.impl.ContainerSchemaNodeBuilder; import org.opendaylight.yangtools.yang.parser.builder.impl.LeafSchemaNodeBuilder; +import com.google.common.base.Objects; +import com.google.common.base.Preconditions; +import com.google.common.base.Splitter; +import com.google.common.base.Strings; +import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; + public class RestconfImpl implements RestconfService { private final static RestconfImpl INSTANCE = new RestconfImpl(); @@ -401,13 +391,36 @@ public class RestconfImpl implements RestconfService { URI rpcNamespace = rpcName.getNamespace(); if (Objects.equal(rpcNamespace.toString(), SAL_REMOTE_NAMESPACE) && Objects.equal(rpcName.getLocalName(), SAL_REMOTE_RPC_SUBSRCIBE)) { - return invokeSalRemoteRpcSubscribeRPC(payload, rpc.getRpcDefinition()); } + validateInput( rpc.getRpcDefinition().getInput(), payload ); + return callRpc(rpc, payload); } + private void validateInput(DataSchemaNode inputSchema, CompositeNode payload) { + if( inputSchema != null && payload == null ) + { + //expected a non null payload + throw new RestconfDocumentedException( "Input is required.", + ErrorType.PROTOCOL, + ErrorTag.MALFORMED_MESSAGE ); + } + else if( inputSchema == null && payload != null ) + { + //did not expect any input + throw new RestconfDocumentedException( "No input expected.", + ErrorType.PROTOCOL, + ErrorTag.MALFORMED_MESSAGE ); + } + //else + //{ + //TODO: Validate "mandatory" and "config" values here??? Or should those be + // validate in a more central location inside MD-SAL core. + //} + } + private StructuredData invokeSalRemoteRpcSubscribeRPC(final CompositeNode payload, final RpcDefinition rpc) { final CompositeNode value = this.normalizeNode(payload, rpc.getInput(), null); @@ -455,8 +468,7 @@ public class RestconfImpl implements RestconfService { throw new RestconfDocumentedException( "Content must be empty.", ErrorType.PROTOCOL, ErrorTag.INVALID_VALUE ); } - final RpcExecutor rpc = resolveIdentifierInInvokeRpc(identifier); - return callRpc(rpc, null); + return invokeRpc( identifier, (CompositeNode)null ); } private RpcExecutor resolveIdentifierInInvokeRpc(final String identifier) { @@ -586,6 +598,9 @@ public class RestconfImpl implements RestconfService { @Override public Response updateConfigurationData(final String identifier, final CompositeNode payload) { final InstanceIdWithSchemaNode iiWithData = this.controllerContext.toInstanceIdentifier(identifier); + + validateInput(iiWithData.getSchemaNode(), payload); + MountInstance mountPoint = iiWithData.getMountPoint(); final CompositeNode value = this.normalizeNode(payload, iiWithData.getSchemaNode(), mountPoint); RpcResult status = null; @@ -610,6 +625,12 @@ public class RestconfImpl implements RestconfService { @Override public Response createConfigurationData(final String identifier, final CompositeNode payload) { + if( payload == null ) { + throw new RestconfDocumentedException( "Input is required.", + ErrorType.PROTOCOL, + ErrorTag.MALFORMED_MESSAGE ); + } + URI payloadNS = this.namespace(payload); if (payloadNS == null) { throw new RestconfDocumentedException( @@ -685,6 +706,12 @@ public class RestconfImpl implements RestconfService { @Override public Response createConfigurationData(final CompositeNode payload) { + if( payload == null ) { + throw new RestconfDocumentedException( "Input is required.", + ErrorType.PROTOCOL, + ErrorTag.MALFORMED_MESSAGE ); + } + URI payloadNS = this.namespace(payload); if (payloadNS == null) { throw new RestconfDocumentedException( diff --git a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/json/to/cnsn/test/JsonToCnSnTest.java b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/json/to/cnsn/test/JsonToCnSnTest.java index 3c70cca0f8..47e329cc3e 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/json/to/cnsn/test/JsonToCnSnTest.java +++ b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/json/to/cnsn/test/JsonToCnSnTest.java @@ -9,8 +9,11 @@ package org.opendaylight.controller.sal.restconf.impl.json.to.cnsn.test; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import java.io.ByteArrayInputStream; +import java.io.InputStream; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -176,6 +179,14 @@ public class JsonToCnSnTest { } + @Test + public void testJsonBlankInput() throws Exception{ + InputStream inputStream = new ByteArrayInputStream( "".getBytes() ); + CompositeNode compositeNode = + JsonToCompositeNodeProvider.INSTANCE.readFrom(null, null, null, null, null, inputStream); + assertNull( compositeNode ); + } + /** * Tests whether namespace stay unchanged if concrete values are * present in composite or simple node and if the method for update is diff --git a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/InvokeRpcMethodTest.java b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/InvokeRpcMethodTest.java index c0c86c3f25..910ca8e20a 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/InvokeRpcMethodTest.java +++ b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/InvokeRpcMethodTest.java @@ -308,7 +308,7 @@ public class InvokeRpcMethodTest { ListenableFuture> mockListener = mock( ListenableFuture.class ); when( mockListener.get() ).thenReturn( rpcResult ); - QName cancelToastQName = QName.create( "cancelToast" ); + QName cancelToastQName = QName.create( "namespace", "2014-05-28", "cancelToast" ); RpcDefinition mockRpc = mock( RpcDefinition.class ); when( mockRpc.getQName() ).thenReturn( cancelToastQName ); diff --git a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPostOperationTest.java b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPostOperationTest.java index ce460fe474..cfbc9fdb76 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPostOperationTest.java +++ b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPostOperationTest.java @@ -149,6 +149,8 @@ public class RestPostOperationTest extends JerseyTest { mockCommitConfigurationDataPostMethod(TransactionStatus.FAILED); assertEquals(500, post(uri, MediaType.APPLICATION_XML, xmlDataInterfaceAbsolutePath)); + + assertEquals( 400, post(uri, MediaType.APPLICATION_JSON, "" )); } @Test @@ -172,6 +174,8 @@ public class RestPostOperationTest extends JerseyTest { assertEquals(204, post(uri, Draft02.MediaTypes.DATA + XML, xmlData4)); uri = "/config/ietf-interfaces:interfaces/interface/0/yang-ext:mount/test-module:cont"; assertEquals(204, post(uri, Draft02.MediaTypes.DATA + XML, xmlData3)); + + assertEquals( 400, post(uri, MediaType.APPLICATION_JSON, "" )); } private void mockInvokeRpc(CompositeNode result, boolean sucessful) { diff --git a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPutOperationTest.java b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPutOperationTest.java index 3af2945526..77b39b7352 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPutOperationTest.java +++ b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/test/RestPutOperationTest.java @@ -22,6 +22,7 @@ import java.util.concurrent.Future; import javax.ws.rs.client.Entity; import javax.ws.rs.core.Application; import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.JerseyTest; @@ -31,6 +32,7 @@ import org.opendaylight.controller.md.sal.common.api.TransactionStatus; import org.opendaylight.controller.sal.core.api.mount.MountInstance; import org.opendaylight.controller.sal.core.api.mount.MountService; import org.opendaylight.controller.sal.rest.impl.JsonToCompositeNodeProvider; +import org.opendaylight.controller.sal.rest.impl.RestconfDocumentedExceptionMapper; import org.opendaylight.controller.sal.rest.impl.StructuredDataToJsonProvider; import org.opendaylight.controller.sal.rest.impl.StructuredDataToXmlProvider; import org.opendaylight.controller.sal.rest.impl.XmlToCompositeNodeProvider; @@ -86,6 +88,7 @@ public class RestPutOperationTest extends JerseyTest { resourceConfig = resourceConfig.registerInstances(restconfImpl, StructuredDataToXmlProvider.INSTANCE, StructuredDataToJsonProvider.INSTANCE, XmlToCompositeNodeProvider.INSTANCE, JsonToCompositeNodeProvider.INSTANCE); + resourceConfig.registerClasses( RestconfDocumentedExceptionMapper.class ); return resourceConfig; } @@ -100,6 +103,15 @@ public class RestPutOperationTest extends JerseyTest { mockCommitConfigurationDataPutMethod(TransactionStatus.FAILED); assertEquals(500, put(uri, MediaType.APPLICATION_XML, xmlData)); + + assertEquals( 400, put(uri, MediaType.APPLICATION_JSON, "" )); + } + + @Test + public void putConfigStatusCodesEmptyBody() throws UnsupportedEncodingException { + String uri = "/config/ietf-interfaces:interfaces/interface/eth0"; + Response resp = target(uri).request( MediaType.APPLICATION_JSON).put(Entity.entity( "", MediaType.APPLICATION_JSON)); + assertEquals( 400, put(uri, MediaType.APPLICATION_JSON, "" )); } @Test diff --git a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/xml/to/cnsn/test/XmlToCnSnTest.java b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/xml/to/cnsn/test/XmlToCnSnTest.java index 5008d28bbf..5cda4a7f52 100644 --- a/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/xml/to/cnsn/test/XmlToCnSnTest.java +++ b/opendaylight/md-sal/sal-rest-connector/src/test/java/org/opendaylight/controller/sal/restconf/impl/xml/to/cnsn/test/XmlToCnSnTest.java @@ -9,8 +9,12 @@ package org.opendaylight.controller.sal.restconf.impl.xml.to.cnsn.test; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import java.io.ByteArrayInputStream; +import java.io.InputStream; + import org.junit.BeforeClass; import org.junit.Test; import org.opendaylight.controller.sal.rest.impl.XmlToCompositeNodeProvider; @@ -52,4 +56,27 @@ public class XmlToCnSnTest extends YangAndXmlAndDataSchemaLoader { assertEquals("121", lf2.getValue()); } + @Test + public void testXmlBlankInput() throws Exception{ + InputStream inputStream = new ByteArrayInputStream( "".getBytes() ); + CompositeNode compositeNode = + XmlToCompositeNodeProvider.INSTANCE.readFrom(null, null, null, null, null, inputStream); + + assertNull( compositeNode ); + } + + @Test + public void testXmlBlankInputUnmarkableStream() throws Exception{ + InputStream inputStream = new ByteArrayInputStream( "".getBytes() ){ + @Override + public boolean markSupported() { + return false; + } + }; + CompositeNode compositeNode = + XmlToCompositeNodeProvider.INSTANCE.readFrom(null, null, null, null, null, inputStream); + + assertNull( compositeNode ); + } + } -- 2.36.6