search

edgexfoundry / edgex-go

EdgeX Golang Services Monorepo | Owner: Core/Support WG
Apache License 2.0
1.33k stars 484 forks source link

Allow Get Command to pass parameters in the payload #3754

Closed cloudxxx8 closed 2 years ago

cloudxxx8 commented 3 years ago

🚀 Feature Request

Relevant Package

This feature request is for core-command ### Description We support URL query parameter to pass into Device Service for Get Command. Sometimes, it is not enough because the URL length is limited, and this would be REST only. ### Describe the solution you'd like Allow Get Command to pass the parameters in the payload to Device Services
farshidtz commented 3 years ago

As discussed in other channels, there is no limit on the URL length. But some web browsers set certain limits.

RFC 2616 Section 3.2.1:

The HTTP protocol does not place any a priori limit on the length of a URI. Servers MUST be able to handle the URI of any resource they serve, and SHOULD be able to handle URIs of unbounded length if they provide GET-based forms that could generate such URIs. A server SHOULD return 414 (Request-URI Too Long) status if a URI is longer than the server can handle (see section 10.4.15).

It should be theoretically possible to encode the object and pass it as query strings. E.g. GET /device/name/{name}/{command}?json=<base64-encoded-object>.

However, passing body in GET is beneficial when the content is too large or sensitive such that having it appeared in logs or a remote logging infrastructural would cause performance or security issues.

With that said, we should be careful when adding that feature:

Hope that helps.

cloudxxx8 commented 3 years ago

@farshidtz thanks for all the information the length limitation of the URI might come from the browsers. json=<base64-encoded-object> is really a workable solution. However, it will be very hard to use by the users. Every command payload should be encoded before calling.

OpenAPI spec is not an issue. We have verified we can use v3.0.3, and the SwaggerHub will allow it. Even if the tools is not allowed payload in the Get method, we can still describe in text in the description field.

I believe this feature can improve the usability and flexibility of the Command Service easily.

cloudxxx8 commented 3 years ago

What I am proposing here is to pass the whole request, including payload and path query parameters to the Device Service implementation. We are right now blocked by the transitional REST paradigm. People have started using GET method with payload for different scenarios. If we execute the command via message bus, the payload will be passed to the Device Service implementation. There is no harm for the existing services and device services.

The scenario I encounter is as followings: To enable more Onvif functions, we need to pass some structural data to the Device. https://www.onvif.org/ver10/device/wsdl/devicemgmt.wsdl image

cloudxxx8 commented 3 years ago

I original thought we should pass whatever payload to the Device Service. However, since Set Command is limited to JSON object, this should also be restricted to the JSON object.

iain-anderson commented 3 years ago

So, are we proposing that as an alternative to using ?param1=xxx&param2=yyy in the URL we should support a JSON payload {"param1":"xxx", "param2":"yyy"} and that this would be equivalent? But that using the payload mechanism would be more convenient especially for large data?

lenny-goodell commented 3 years ago

@iain-anderson , yes. When the objects are more complex JSON when nested types it is nasty to try to put them in a query parameter.

cloudxxx8 commented 3 years ago

@iain-anderson the purpose of them are the same, they are both provide a means to send out additional parameters to Get Commands.

farshidtz commented 3 years ago

@farshidtz thanks for all the information the length limitation of the URI might come from the browsers. json=<base64-encoded-object> is really a workable solution. However, it will be very hard to use by the users. Every command payload should be encoded before calling.

OpenAPI spec is not an issue. We have verified we can use v3.0.3, and the SwaggerHub will allow it. Even if the tools is not allowed payload in the Get method, we can still describe in text in the description field.

I believe this feature can improve the usability and flexibility of the Command Service easily.

I'm not sure how this would affect the usability for users in a browser. I guess you have a front-end web application which takes the input from a user and wants to pass it along in the body instead of URL. If that's the case, the difference is really an encoding step within the application.

Putting in description is not enough because the OpenAPI spec may be used for client stub generation and testing. But this not a big deal since tools will get updated sooner or later. We just need to make sure we update the spec's OpenAPI version to one that officially supports GET requestBody.

Another issue that comes to mind is caching. It looks like Kong's proxy cache has no way of taking the body into consideration in the cache control key. So if caching is enabled, subsequent GET requests to the same endpoint (with different bodies) may hit the cache and return unexpected identical results. This should be documented somewhere to inform the users.

iain-anderson commented 3 years ago

Would be nice if we can find a better example - the documentation for GetPkcs10Request looks to be wrong and it is also just strings.

iain-anderson commented 3 years ago

Also for future (v3?) consideration - This looks as though the device has a number of similar resources (certificates) with names that we do not know in advance, so cannot include in a profile. It would be good to be able to model scenarios such as this. For example:

tonyespy commented 3 years ago

The first thing I want to address is that in several places @cloudxxx8 mentions limitations due to REST.

In the issue's description:

Sometimes, it is not enough because the URL length is limited, and this would be REST only.

And later on:

We are right now blocked by the transitional REST paradigm. People have started using GET method with payload for different scenarios. If we execute the command via message bus, the payload will be passed to the Device Service implementation.

The last time I checked, EdgeX 2.x only supports device commands via REST. Could we use the message bus some day? Yes we could, but today REST is what's supported for command actuation.

Second, I'd like to point out that this is not just a change to Core Command, this is a cross-cutting feature enhancement. Changes would also need to be made to the device SDKs, including the public APIs the SDKs define for passing commands back and forth to device service protocol implementations. Note, the changes required are additions only, so at least what's being proposed doesn't break backwards compatibility.

Third, the example given is the key to this whole discussion. The ONVIF protocol is based on open Web Services standards such as SOAP, XML, and WSDL. SOAP provides a framework for functional APIs whereas REST provides a framework for data focused APIs. Also as an aside, looking over the functions defined in the ONVIF device services, I don't see any functions that define so many parameters that it wouldn't be feasible to encode via the query string. I think the bigger issue is dealing with allowable characters in the URI.

What @cloudxxx8 is proposing a way to use SOAP-based services over REST (again, please remember this is all we support today, so let's please leave message buses out of this discussion wrt to Jakarta).

As mentioned in the Core WG meeting I had a number of questions about the current proposal, which frankly is still lean on details. If we're going to land this, I would prefer that the description of this issue be edited and the details added there, as opposed to scattering the details across one or more comments.

Finally, please remember, Jakarta is an LTS release, which means we need to support whatever we come up with for two years.

cloudxxx8 commented 2 years ago

@tonyespy thanks for the comments. I will modify the description once we get the conclusion.

  • Can the caller pass any content type or do we restrict these payload parameters to JSON? It seems @cloudxxx8 has answered in one of his comments that we should restrict to JSON only.
  • Is this restricted to Get/Read commands that specify device resources? I don't see how we could support multiple bodies if the specified source was a device command?

No, it should not be restricted. The Device Resource or Device Command are related to the return values, not request values. In this Get command payload, it's a convenient way to define a complex query conditions or security tokens. Core Command or Device SDK should just pass them to the Device Service implementation as the URL query parameters.

  • Should we restrict this to device resources with valueType=object?

No, it is for response type. It's doesn't impact the request body.

  • I think what's also being proposed is that the device resource would specify the function/service being invoked. Is that correct @cloudxxx8? If so, @lenny-intel and I discussed the fact that the existing device-camera service does this, and we both agreed it shouldn't really be considered a best practice due fragility (e.g. the breakage that happened with device-camera 2.0 and the camel case changes to the its device profiles).

It's correct. The device resource which needs complex query conditions is more like a function. I agree. It shouldn't be a best practice.

  • Should we really let clients use Gets/Reads to call services that update data on the device and/or trigger actions? If so, again that's not very REST-like. If not, then are we proposing something similar for the Put/Write method?

The problem of Put/Write method is that there is no response body. The Get commands we are discussing is more like a query, so it needs complex query conditions and complex responses. We might also encounter the issue of requiring complex response body of Put/Write method in the future.

  • Have we considered any other alternative solutions? For instance, could define an dedicated endpoint for this style API? Have we looked at other REST-based projects which have done something similar?

Additional dedicated endpoint might be more complicated solution. I saw the REST Get method with body usage in a product internal API when I worked in IBM. Here is another example: https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-body.html

cloudxxx8 commented 2 years ago

@farshidtz thanks for all the information the length limitation of the URI might come from the browsers. json=<base64-encoded-object> is really a workable solution. However, it will be very hard to use by the users. Every command payload should be encoded before calling. OpenAPI spec is not an issue. We have verified we can use v3.0.3, and the SwaggerHub will allow it. Even if the tools is not allowed payload in the Get method, we can still describe in text in the description field. I believe this feature can improve the usability and flexibility of the Command Service easily.

I'm not sure how this would affect the usability for users in a browser. I guess you have a front-end web application which takes the input from a user and wants to pass it along in the body instead of URL. If that's the case, the difference is really an encoding step within the application.

Putting in description is not enough because the OpenAPI spec may be used for client stub generation and testing. But this not a big deal since tools will get updated sooner or later. We just need to make sure we update the spec's OpenAPI version to one that officially supports GET requestBody.

Another issue that comes to mind is caching. It looks like Kong's proxy cache has no way of taking the body into consideration in the cache control key. So if caching is enabled, subsequent GET requests to the same endpoint (with different bodies) may hit the cache and return unexpected identical results. This should be documented somewhere to inform the users.

Thanks, @farshidtz you are right. There should be an application to build a path or request body normally. However, for the demo purpose, we can use Postman to achieve the request body easily. It needs some extra steps to encode and decode a base64 string. About Kong cache, we have to write the document to remind users not to enable it. It's the same no mater with or without the request body for Core Commands.

cloudxxx8 commented 2 years ago

@iain-anderson ok, here is another example: https://www.onvif.org/ver10/media/wsdl/media.wsdl

image

cloudxxx8 commented 2 years ago

According to the discussion in the Device WG meeting, decide to allow URL path parameter only. Close this issue.