Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: suport complex objects for query params in api explorer #4347

Merged
merged 1 commit into from Feb 6, 2020

Conversation

@deepakrkris
Copy link
Contributor

deepakrkris commented Dec 30, 2019

issue: #2208

BREAKING CHANGE

Based on spike #4141

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated
@deepakrkris deepakrkris requested review from bajtos and raymondfeng as code owners Dec 30, 2019
@deepakrkris deepakrkris force-pushed the apiExpQObj branch 4 times, most recently from 084cbcc to 75601f7 Dec 30, 2019
@deepakrkris deepakrkris requested review from agnes512 and jannyHou Dec 31, 2019
Copy link
Contributor

agnes512 left a comment

According to the spike, the implementation part LGTM. Can we have some tests to check the changes and different encoding styles?


if (!schema && spec.content) {
const content = spec.content;
const jsonSpec = content['application/json'];

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jan 2, 2020

Contributor

@deepakrkris The encoding type should be inferred from the request context, coerceParameter is called here, which has access to the request.

So I would suggest add a 3rd parameter options for coerceParameter, and provide the request as options.reqCtx(or some better name), then retrieve the encoding type from request.

If schema found then use it, if no schema field, search for spec.content[encodingType].

This comment has been minimized.

Copy link
@deepakrkris

deepakrkris Jan 24, 2020

Author Contributor

@jannyHou , 'application/json' here does not refer to the incoming request's encoding type. It tells what type of content a particular parameter carries. As per Open API spec 3 , in:query and content: application/json: schema together means the query parameter is a json encoded string.

This comment has been minimized.

Copy link
@deepakrkris

deepakrkris Jan 24, 2020

Author Contributor

@jannyHou we can safely look for only application/json in the content field for condition in: query , because Open API v3 supports only this encoding type inside content field for a query parameter.

Copy link
Member

bajtos left a comment

@deepakrkris we will need to clean up the commits a bit before this PR can be landed.

First of all, please take a look at our commit message guidelines to better understand how to describe breaking changes in the commit messages. Secondly, read Making breaking changes to learn what information should be provided in the "BREAKING CHANGES" footer.

It's probably best to leave this cleanup to the end, once the code (and documentation?) changes are approved.

BTW, in the checklist, you are indicating that the following steps were taken:

  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

However, I don't see any such changes in your pull request. Please take a look.

@deepakrkris deepakrkris force-pushed the apiExpQObj branch 9 times, most recently from b23b6c6 to 65158b4 Jan 23, 2020
1 Outdated Show resolved Hide resolved
@deepakrkris deepakrkris force-pushed the apiExpQObj branch 4 times, most recently from 4e8fe35 to 61dd6df Jan 23, 2020
spec.in === 'query' &&
spec.content &&
spec.content['application/json'] &&
spec.content['application/json'].schema

This comment has been minimized.

Copy link
@raymondfeng

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jan 27, 2020

Contributor

+1 for Raymond's comment, try

if (spec.content?.['application/json']?.schema) {
//
}

?

This comment has been minimized.

Copy link
@deepakrkris

deepakrkris Jan 29, 2020

Author Contributor

@raymondfeng @jannyHou I have implemented as per your suggestions with optional chaining. please verify.

@deepakrkris

This comment has been minimized.

Copy link
Contributor Author

deepakrkris commented Jan 24, 2020

According to the spike, the implementation part LGTM. Can we have some tests to check the changes and different encoding styles?

@agnes512 I have added various test cases. Could you please verify and comment.

@deepakrkris deepakrkris self-assigned this Jan 24, 2020
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Jan 29, 2020

@deepakrkris In the commit message for cd3b772, we should add more details about the BREAKING CHANGE to explain:

  1. What's breaking?
  2. What do current users have to adjust?
@deepakrkris deepakrkris force-pushed the apiExpQObj branch from 9cb699c to ae9980e Jan 30, 2020
@deepakrkris

This comment has been minimized.

Copy link
Contributor Author

deepakrkris commented Jan 30, 2020

@deepakrkris In the commit message for cd3b772, we should add more details about the BREAKING CHANGE to explain:

  1. What's breaking?
  2. What do current users have to adjust?

@raymondfeng @bajtos updated the commit message with the BREAKING CHANGE description

BREAKING CHANGE: This fix changes the schema definition of query parameters with `in: query`
and `style: deepObject` by moving the `schema` field under `content[application/json]`,
i.e, json query params are changed to accomodate url encoding as per open api specifications.
The `style` and `explode` fields are also removed for such query parameters`.
The signature of the `param.query.object` decorator has not changed due to this fix.
There is no code changes required after upgrading this fix. No method signatures or
data structures are impacted. The impact is only on the open api specification generated
from the LoopBack APIs with json query params.
All clients and consumers of such APIs may have to regenerate the open api specifications.
@deepakrkris deepakrkris force-pushed the apiExpQObj branch 3 times, most recently from 87c8a41 to 206edb2 Jan 30, 2020
@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Jan 31, 2020

@deepakrkris Can you squash the commits into one?

Copy link
Contributor

emonddr left a comment

Squash commits as @raymondfeng suggests

Copy link
Contributor

jannyHou left a comment

LGTM and yes please make sure ALL the changes are squashed into the commit that contains breaking change note.

Copy link
Member

bajtos left a comment

Thank you @deepakrkris for writing detailed release notes about the breaking change. When I read it with a fresh eyes, I find the first sentence a bit confusing:

This fix changes the schema definition of query parameters with in: query
and style: deepObject by moving the schema field under content[application/json]

In a typical application, users are calling @param.query.object() decorator and don't deal with low-level OpenAPI spec details.

The text also creates an impression that LoopBack is going to modify the OpenAPI spec even when the user has explicitly decided to describe a parameter as style: deepObject, for example using generic @param decorator or even using a design-first approach where the entire OpenAPI spec is provided from outside via app.api() method, see Defining the API using design-first approach.

I would find such behavior problematic:

  • We are changing OpenAPI spec fragment written manually by the app developer.
  • If a parameter is not described via our @param decorator, then I would assume the developer has good reasons why they are providing the spec explicitly and thus we should honor it as-is.
  • If we silently change the spec, then it will be difficult for app developers to find out what's happening to their spec, why is it being changed.
  • It is no longer possible to force the parameter to be described as style: deepObject, because the framework will always convert such spec to JSON-encoded style.

I checked the implementation and IIUC it works differently, it introduces the expected behavior - param.query.object() emits a different OpenAPI spec fragment.

Let's improve the description of the breaking change to make this more clear. I think it's important to start from the point of view of a developer using @param.query.object() decorator.

For example (I added emphasis to highlight my changes):

BREAKING CHANGE: This fix changes the OpenAPI schema definition generated by the decorator param.query.object. Before this change, such parameters were described with style: deepObject encoding, which turned out to be problematic, as explained and discussed in swagger-api/swagger-js#1385. To address these issues, we switched the parameter definition to JSON encoding by moving the schema field under content[application/json], i.e, json query params are changed to accommodate url encoding as per open api specifications. The style and explode fields are also removed for such query parameters`.

The signature of the param.query.object decorator API has not changed due to this fix. There is no code changes required after upgrading this fix. No method signatures or data structures are impacted. The impact is only on the open api specification generated from the LoopBack APIs with json query params.

To preserve compatibility with existing REST API clients, request parameters encoded using deep-object notation are still parsed as before, despite the fact that they are described differently in the OpenAPI spec. As a result, existing REST API clients will keep working after the upgrade of the LoopBack server runtime, no changes are required.

All clients and consumers of such APIs may have to regenerate the open api specifications.

@@ -172,9 +179,9 @@ function parseJsonIfNeeded(
): string | object | undefined {
if (typeof data !== 'string') return data;

if (spec.in !== 'query' || spec.style !== 'deepObject') {
if (spec.in !== 'query' || (spec.in === 'query' && !spec.content)) {

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 3, 2020

Member

IIUC, this is introducing another breaking change. If I am reading the code right:

  • Before this change, when a parameter was described with style: deepObject, we attempted to parse it as JSON.
  • After this change, when a parameter is described with style: deepObject, we never parse the value as JSON and require the value to be encoded using deep query string.

While the new behavior is technically more correct, I am little bit concerned about the impact on the existing applications. I understand this does not apply to apps using @param.query.object decorator, which is probably the majority of the apps out there. However, it is still possible to specify style: deepObject encoding for parameters, for example using generic @param decorator or even using a design-first approach where the entire OpenAPI spec is provided from outside via app.api() method, see Defining the API using design-first approach.

If don't have strong opinions on whether preserve old (more lenient) behavior or introduce the new (more strict) handling. If we decide to be more strict, then please make sure this change is spelled out in the release notes (the commit message).

This comment has been minimized.

Copy link
@bajtos

bajtos Feb 3, 2020

Member

On the second thought, I think now is a good time to introduce this breaking change, since we are already changing other things. It should be easy (and backwards compatible) to reintroduce support for JSON-encoded deep-object parameters in the future, assuming there is anybody asking for that feature.

So the only remaining step is to update the commit message and explain the breaking change.

This comment has been minimized.

Copy link
@deepakrkris

deepakrkris Feb 3, 2020

Author Contributor
  • After this change, when a parameter is described with style: deepObject, we never parse the value as JSON and require the value to be encoded using deep query string.

@bajtos share your concern on the above , but as per this change there is no decorator to annotate a parameter with style:deepObject so there is no way a customer can have a case of trying to send a complex json for a parameter marked with spec deepObject.

This comment has been minimized.

Copy link
@deepakrkris

deepakrkris Feb 4, 2020

Author Contributor
  • After this change, when a parameter is described with style: deepObject, we never parse the value as JSON and require the value to be encoded using deep query string.

for the change in this behavior I have added the following in the commit footer:

This fix has also modified the coercion behaviour for `in:query`, `deep-object` query
parameters by removing json parsing for them and the expectation is to pass in only
string values. But since the `exploded` `deep-object` style definition for query params
has been removed, this behaviour remains just an internal source code aspect as of now.
@bajtos bajtos dismissed their stale review Feb 3, 2020

The important concerns have been addressed.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Feb 3, 2020

BTW, it makes me wonder if there are any changes needed in these areas, to ensure they stay in sync with the actual runtime behavior?

  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

For example, if we have any documentation page showing an example of an OpenAPI spec document (or a fragment) generated by LoopBack, then we need to update it to reflect the changes in the runtime.

A quick string search for deepObject found the following place that I think needs an update: https://github.com/strongloop/loopback-next/blob/75ba77d5bdfe24f612c418c9100fa7e008211709/docs/site/Parsing-requests.md#object-values, see the full content below the line.


OpenAPI specification describes several ways how to encode object values into a
string, see Style Values and Style Examples.

At the moment, LoopBack supports object values for parameters in query strings with style: "deepObject" only. Please note that this style does not preserve encoding of primitive types, numbers and booleans are always parsed as strings.

For example:

GET /todos?filter[where][completed]=false
// filter={where: {completed: 'false'}}

As an extension to the deep-object encoding described by OpenAPI, when the parameter is specified with style: "deepObject", we allow clients to provide the object value as a JSON-encoded string too.

For example:

GET /todos?filter={"where":{"completed":false}}
// filter={where: {completed: false}}
@deepakrkris deepakrkris force-pushed the apiExpQObj branch 7 times, most recently from d37446d to cf72abd Feb 3, 2020
@deepakrkris

This comment has been minimized.

Copy link
Contributor Author

deepakrkris commented Feb 4, 2020

A quick string search for deepObject found the following place that I think needs an update: https://github.com/strongloop/loopback-next/blob/75ba77d5bdfe24f612c418c9100fa7e008211709/docs/site/Parsing-requests.md#object-values, see the full content below the line.

OpenAPI specification describes several ways how to encode object values into a
string, see Style Values and Style Examples.

At the moment, LoopBack supports object values for parameters in query strings with style: "deepObject" only. Please note that this style does not preserve encoding of primitive types, numbers and booleans are always parsed as strings.

@bajtos I will create a separate PR to update for this. Thanks for bringing this to my notice.

@deepakrkris deepakrkris force-pushed the apiExpQObj branch 2 times, most recently from c1cfb8d to ffcbb3d Feb 4, 2020
BREAKING CHANGE: This fix has modified the api definitions described by the decorator
'param.query.object', to support Open-API's `url-encoded` definition for json query
parameters.

Previously, such parameters were described with `exploded: true` and
`style: deepObject`, i.e exploded encoding, which turned out to be problematic as explained and discussed in,
swagger-api/swagger-js#1385 and
OAI/OpenAPI-Specification#1706

```json
  {
    "in": "query",
    "style": "deepObject"
    "explode": "true",
    "schema": {}
  }
```

Exploded encoding worked for simple json objects as below but not for complex objects.

```
   http://localhost:3000/todos?filter[limit]=2
```

To address these issues with exploded queries, this fix switches definition of json
query params from the `exploded`, `deep-object` style to the `url-encoded` style
definition in Open-API spec.

LoopBack already supports receiving url-encoded payload for json query parameters.

For instance, to filter api results from the GET '/todo-list' endpoint in the
todo-list example with a specific relation, { "include": [ { "relation": "todo" } ] },
the following url-encoded query parameter can be used,

```
   http://localhost:3000/todos?filter=%7B%22include%22%3A%5B%7B%22relation%22%3A%22todoList%22%7D%5D%7D
```

The above was possible because the coercion behavior in LoopBack performed json
parsing for `deep object` style json query params before this fix. This fix has
modified that behavior by removing json parsing. Since the `exploded` `deep-object`
definition has been removed from the `param.query.object` decorator, this new
behaviour remains just an internal source code aspect as of now.

In effect, this fix only modifies the open api definitions generated from LoopBack
APIs. The 'style' and 'explode' fields are removed and the 'schema' field is moved
under 'content[application/json]'. This is the definition that supports url-encoding
as per Open-API spec.

```json
  {
    "in": "query"
    "content": {
      "application/json": {
        "schema": {}
      }
    }
  }
```

Certain client libraries (like swagger-ui or LoopBack's api explorer) necessiate
using Open-API's `url-encoded` style definition for json query params to support
"sending" url-encoded payload.

All consumers of LoopBack APIs may need to regenerate api definitions, if their
client libraries require them to do so for url-encoding.

Otherwise there wouldn't be any significant impact on API consumers.

To preserve compatibility with existing REST API clients, this change is backward
compatible. All exploded queries like `?filter[limit]=1` will continue to work for
json query params, despite the fact that they are described differently in the
OpenAPI spec.

Existing api clients will continue to work after an upgrade.

The signature of the 'param.query.object' decorator has not changed.

There is no code changes required in the LoopBack APIs after upgrading to this
fix. No method signatures or data structures are impacted.
@deepakrkris deepakrkris force-pushed the apiExpQObj branch from ffcbb3d to 4ff23e6 Feb 4, 2020
@dhmlau

This comment has been minimized.

Copy link
Member

dhmlau commented Feb 5, 2020

@slnode test please

@bajtos
bajtos approved these changes Feb 6, 2020
@deepakrkris deepakrkris merged commit a4ef640 into master Feb 6, 2020
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls First build on apiExpQObj at 92.429%
Details
@deepakrkris deepakrkris deleted the apiExpQObj branch Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can鈥檛 perform that action at this time.