feat: spike on API Explorer supporting complex objects in query param #4141

deepakrkris · 2019-11-18T03:50:08Z

Scope:

Currently loopback supports both exploded and uri encoded complex objects in query params, even without the changes suggested in this PR. The purpose of this PR is to ONLY fix API Explorer to support complex objects in query params.

To test this spike,

Install an example app (eg: todoList)
change the param decorators for the filter query in a controller method to @param.query.jsonObject
run npm build and copy the installed example app into this PR branch -> loopback-next/sandbox directory
run npm i under loopback-next , to create symlinks
cd sandbox/loopback4-example-todoList && npm run start
goto API Explorer http://localhost:3000/explorer
check invoking controller method that has @param.query.jsonObject works when a filter object is passed

deepakrkris · 2019-11-19T06:06:51Z

bajtos · 2019-11-19T07:28:30Z

Thank you for providing test instruction and a screenshot of the outcome. Just FYI, the instructions could be simplified by taking one of our example applications (e.g. examples/todo) and changing the param decorators for the filter query in that app.

bajtos · 2019-11-19T07:30:17Z

It's great to see that swagger-ui can handle json-encoded query parameters. Now we need to carefully consider backwards compatibility, see #3770 (comment).

An important aspect to consider: if we switch the spec of format argument from deepObject-style to json-content, how is it going to affect the actual REST API? Will it mean that clients will be no longer able to send queries like find?filter[limit]=1 and will have to always use JSON encoding only (find?filter={"limit":1})? Can we find a way how to preserve support for both flavors?

bajtos · 2019-11-19T07:33:20Z

examples/todo/src/__tests__/acceptance/todo.acceptance.ts

@@ -189,7 +189,7 @@ describe('TodoApplication', () => {

    await client
      .get('/todos')
-      .query({filter: {where: {isComplete: false}}})
+      .query({filter: encodeURIComponent(JSON.stringify({where: {isComplete: false}})) })


Ideally, both flavors should be supported:

filter: {where: {isComplete: false}}

filter: encodeURIComponent(JSON.stringify(...))

Instead of changing this existing test, we should add a new test to verify the other flavor. I believe these two flavors are already supported now, in which case it would be great to have tests to verify all different ways how to provide the filter argument.

(This code change & discussion thread is directly related to my previous comment about backwards compatibility.)

An idea to consider & try: can we mix deepObject style with json style in a single parameter? Something along the following lines:

object: function( name: string, schema: SchemaObject | ReferenceObject = { type: 'object', additionalProperties: true, }, spec?: Partial<ParameterObject>, ) { schema = { type: 'object', ...schema, }; return param({ name, in: 'query', // deepObject style: 'deepObject', explode: true, // new addition - json encoding content: { "application/json": { schema } }, // the rest schema, ...spec, }); },

+1 Let's make sure the default signature filter: {where: {isComplete: false}} still works.

Ideally, both flavors should be supported:

filter: {where: {isComplete: false}}

filter: encodeURIComponent(JSON.stringify(...))

@bajtos while using super-test both the above are the same , ie, using encodeURIComponent(JSON.stringify( for super test is redundant, while for other http client invocations it is required. sending just filter: {where: {isComplete: false}} in url will not work.
I suppose you are asking to support exploded query params like filter[limit]=1

Instead of changing this existing test, we should add a new test to verify the other flavor. I believe these two flavors are already supported now, in which case it would be great to have tests to verify all different ways how to provide the filter argument.

yes both the exploded and complex objects are already supported in loopback, even without the changes in this PR. The suggested changes in this PR is only for API Explorer to support complex objects.

@bajtos @jannyHou I have now implemented your suggestions to modify existing decorator with backward compatibility, and also support complex objects for query params in API explorer.

@bajtos @jannyHou , to be clear , currently loopback supports both exploded and uri encoded complex objects in query params, even without the changes suggested in this PR. The purpose of this PR is to ONLY fix API Explorer to support complex objects in query params.

@bajtos @jannyHou , I have tested , for example apps API Explorer is working fine now for complex objects in query params.

bajtos · 2019-11-19T07:35:40Z

Last but not least, please check why Travis CI is failing and try to make all tests pass.

I see the following compilation error, there is also a linting error.

packages/openapi-v3/src/decorators/parameter.decorator.ts(252,9): error TS1005: ',' expected.

jannyHou

@deepakrkris Great findings! commented. The solution is elegant and LGTM in general.

jannyHou · 2019-11-19T21:33:30Z

packages/openapi-v3/src/decorators/parameter.decorator.ts

+     * @param name - Parameter name
+     * @param schema - Optional OpenAPI Schema describing the JSON object.
+     */
+    jsonObject: function(


The solution is cool!
Implementation details...I think we don't need a new decorator to support the multi contents, @bajtos 's comment is sth we could try https://github.com/strongloop/loopback-next/pull/4141/files#r347767523.

Or provide the content object as the 3rd parameter spec?: Partial<ParameterObject>

@deepakrkris You may want to check is it a valid spec to have schema and content both defined, like:

// new addition - json encoding content: { "application/json": { schema } }, // the rest // @jannyHou: Is it valid to have both `schema` and `content` defined? schema,

And I feel the new signature of param.query.object() would be very similar to @requestBody, which reminds me to have a chat with @bajtos on #3493, I will schedule a meeting asap and update our conclusion here. Anyway the spike is good enough to me :) We can determine the signature in the follow-up stories.

@jannyHou I have modified existing decorator with backward compatibility and also fix API Explorer for complex objects in query params.

jannyHou · 2019-11-19T21:37:22Z

examples/todo/src/__tests__/acceptance/todo.acceptance.ts

@@ -189,7 +189,7 @@ describe('TodoApplication', () => {

    await client
      .get('/todos')
-      .query({filter: {where: {isComplete: false}}})
+      .query({filter: encodeURIComponent(JSON.stringify({where: {isComplete: false}})) })


+1 Let's make sure the default signature filter: {where: {isComplete: false}} still works.

jannyHou

Great, 👍

bajtos

I love this solution too ❤️

Do we have any automated tests to verify that our applications are emitted valid OpenAPI spec documents? Can you please double-check?

From what I remember, there are two OpenAPI validators and sometimes one of them reports error for spec documents that are considered as valid by the other.

When I paste the spec for examples/todo-list to Swagger Editor (see https://editor.swagger.io), I get several "structural errors" reported, presumably one for each use of the new query param object style:

Structural error at paths./todos.get.parameters.0
should have either a `schema` or `content` property
Jump to line 868

The same error is reported by swagger validator (see https://apitools.dev/swagger-parser/online/):

Swagger schema validation failed. 
  Data does not match any schemas from 'oneOf' at #/paths//todos/get/parameters/0
    Data matches schema from 'not' at #/paths//todos/get/parameters/0
    Missing required property: $ref at #/paths//todos/get/parameters/0
 
JSON_OBJECT_VALIDATION_FAILED

See also https://github.com/OAI/OpenAPI-Specification/blob/3.0.1/versions/3.0.1.md#parameter-object, emphasis is mine:

The rules for serialization of the parameter are specified in one of two ways. For simpler scenarios, a schema and style can describe the structure and syntax of the parameter.
(...)
For more complex scenarios, the content property can define the media type and schema of the parameter. A parameter MUST contain either a schema property, or a content property, but not both. When example or examples are provided in conjunction with the schema object, the example MUST follow the prescribed serialization strategy for the parameter.

Let's find a way how to emit OpenAPI spec that's valid.

bajtos · 2019-11-21T10:33:44Z

examples/todo-list/src/__tests__/acceptance/todo-list.acceptance.ts

+    await givenTodoInstance(todoRepo, {title: 'todo2', todoListId: list.id});
+    await givenTodoInstance(todoRepo, {title: 'todo3', todoListId: list.id});
+
+    const response = await client.get('/todos').query('filter[limit]=2');


This is a bit redundant, because query({filter: {limit: 2}}) is automatically converted by supertest to ?filter[limit]=2. Having said that, I think it's a good idea to have tests that are explicitly verifying different encoding styles, as opposed to the current status, where such encodings are tested implicitly, see:

https://github.com/strongloop/loopback-next/blob/303d3ff6ebe751b238ab16e021f6a25f09f4a1be/examples/todo-list/src/__tests__/acceptance/todo-list.acceptance.ts#L110

https://github.com/strongloop/loopback-next/blob/303d3ff6ebe751b238ab16e021f6a25f09f4a1be/examples/todo-list/src/__tests__/acceptance/todo-list.acceptance.ts#L196-L198

Writing such tests is out of scope of this spike, but please do include a task/todo-item in the acceptance criteria of the story that will track the real implementation.

bajtos · 2019-11-21T14:01:27Z

Considering that runtime supports both deepObject and json encodings (no changes are needed to fix API Explorer experience), and many of our users (hopefully?) don't care that much how object parameters are described in the spec, as long as it's a valid spec that's supported by API Explorer, I am leaning towards removing deepObject style from the generating spec and providing content only.

This can break existing applications (for example when using clients code-generated by a tool that may not support json encoding), therefore this change needs to be published as semver-major of @loopback/openapi-v3 and @loopback/rest.

Thoughts?

/cc @strongloop/loopback-next

jannyHou · 2019-11-25T16:11:51Z

@bajtos I am glad you verified manually that schema and content has conflict and found the related doc, good catch!

I am leaning towards removing deepObject style from the generating spec and providing content only.

Sounds good to me.

This can break existing applications (for example when using clients code-generated by a tool that may not support json encoding), therefore this change needs to be published as semver-major of @loopback/openapi-v3 and @loopback/rest.

Fair enough to have a semver-major release to support querying nested filter.

deepakrkris · 2019-11-27T03:37:38Z

@bajtos @jannyHou , I have fixed the parameter spec. removed style , explode and schema fields. Now schema passes https://editor.swagger.io/ . But there are additional changes required in loopback to support extracting param schema from content field. Tested with API Explorer and browser, works fine.

deepakrkris · 2019-11-27T03:51:08Z

@bajtos I have also removed the explode and style properties because https://editor.swagger.io/ complains for them as well (when content field is included in the schema)

deepakrkris · 2019-11-27T19:04:06Z

All the suggestions are implemented and tested and the CI passes, so I am closing this PR. findings of this spike are updated in the spike PR as well as the linked original issue.

bajtos · 2019-11-28T16:25:18Z

packages/rest/src/coercion/coerce-parameter.ts

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


This is ok in a spike, but please note that the real implementation must not mutate (modify) the spec object given to us by the caller.

deepakrkris requested review from bajtos and raymondfeng as code owners November 18, 2019 03:50

deepakrkris force-pushed the spikeUrlEncQueryParam branch from b9d1dc3 to 7f6f73d Compare November 18, 2019 04:07

deepakrkris mentioned this pull request Nov 18, 2019

[Spike] Investigate how to allow complex object in filter object in API Explorer #3770

Closed

1 task

deepakrkris force-pushed the spikeUrlEncQueryParam branch 3 times, most recently from 38e4f3e to 2649c50 Compare November 19, 2019 05:49

deepakrkris self-assigned this Nov 19, 2019

bajtos reviewed Nov 19, 2019

View reviewed changes

ewrayjohnson mentioned this pull request Nov 19, 2019

API Explorer ignores filter options where, fields, and order #2208

Closed

7 tasks

deepakrkris force-pushed the spikeUrlEncQueryParam branch from 2649c50 to 91980b6 Compare November 19, 2019 18:20

jannyHou reviewed Nov 19, 2019

View reviewed changes

deepakrkris force-pushed the spikeUrlEncQueryParam branch 6 times, most recently from cd00442 to 303d3ff Compare November 20, 2019 04:46

deepakrkris changed the title ~~feat: spike on url encoding of json object in query param~~ feat: spike on API Explorer supporting complex objects in query param Nov 20, 2019

raymondfeng approved these changes Nov 20, 2019

View reviewed changes

jannyHou approved these changes Nov 20, 2019

View reviewed changes

bajtos suggested changes Nov 21, 2019

View reviewed changes

jannyHou approved these changes Nov 27, 2019

View reviewed changes

deepakrkris force-pushed the spikeUrlEncQueryParam branch 2 times, most recently from 3355247 to 5604f7b Compare November 27, 2019 17:43

deepakrkris added 2 commits November 27, 2019 10:21

feat: spike on url encoding of json object in query param

2a763f7

fix: remove style and schema from param spec

9218d22

deepakrkris force-pushed the spikeUrlEncQueryParam branch from 5604f7b to 9218d22 Compare November 27, 2019 18:22

deepakrkris closed this Nov 27, 2019

bajtos reviewed Nov 28, 2019

View reviewed changes

deepakrkris mentioned this pull request Dec 31, 2019

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

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: spike on API Explorer supporting complex objects in query param #4141

feat: spike on API Explorer supporting complex objects in query param #4141

deepakrkris commented Nov 18, 2019 •

edited

Loading

deepakrkris commented Nov 19, 2019

bajtos commented Nov 19, 2019

bajtos commented Nov 19, 2019

bajtos Nov 19, 2019

bajtos Nov 19, 2019

bajtos Nov 19, 2019

jannyHou Nov 19, 2019

deepakrkris Nov 20, 2019

deepakrkris Nov 20, 2019 •

edited

Loading

deepakrkris Nov 20, 2019

deepakrkris Nov 20, 2019

bajtos commented Nov 19, 2019

jannyHou left a comment

jannyHou Nov 19, 2019 •

edited

Loading

jannyHou Nov 19, 2019

deepakrkris Nov 20, 2019

jannyHou Nov 19, 2019

jannyHou left a comment

bajtos left a comment

bajtos Nov 21, 2019

bajtos commented Nov 21, 2019

jannyHou commented Nov 25, 2019 •

edited

Loading

deepakrkris commented Nov 27, 2019

deepakrkris commented Nov 27, 2019

deepakrkris commented Nov 27, 2019

bajtos Nov 28, 2019

feat: spike on API Explorer supporting complex objects in query param #4141

feat: spike on API Explorer supporting complex objects in query param #4141

Conversation

deepakrkris commented Nov 18, 2019 • edited Loading

Scope:

deepakrkris commented Nov 19, 2019

bajtos commented Nov 19, 2019

bajtos commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepakrkris Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 19, 2019

jannyHou left a comment

Choose a reason for hiding this comment

jannyHou Nov 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 21, 2019

jannyHou commented Nov 25, 2019 • edited Loading

deepakrkris commented Nov 27, 2019

deepakrkris commented Nov 27, 2019

deepakrkris commented Nov 27, 2019

Choose a reason for hiding this comment

deepakrkris commented Nov 18, 2019 •

edited

Loading

deepakrkris Nov 20, 2019 •

edited

Loading

jannyHou Nov 19, 2019 •

edited

Loading

jannyHou commented Nov 25, 2019 •

edited

Loading