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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 task done
dhmlau opened this issue Sep 19, 2019 · 11 comments
Closed
1 task done
Assignees
Labels
developer-experience Issues affecting ease of use and overall experience of LB users spike

Comments

@dhmlau
Copy link
Member

dhmlau commented Sep 19, 2019

Originated from #2208.

Description

swagger-ui, the library powering our API Explorer, unfortunately does not support complex objects in query strings. This spike is to investigate if/how we can allow it.

Extracting from #2208 (comment) from @bajtos

Based on OAI/OpenAPI-Specification#1706 (comment) (cross-posted below), maybe we can switch filter encoding from deepObject style into JSON?

Here is an experiment we can try:

  1. Let's keep @param.query.object with no changes for backwards compatibility.
  2. Introduce a new helper @param.query.jsonObject that will use JSON encoding instead of deepObject style.
  3. Modify the function parseJsonIfNeeded to detect JSON encoding and parse the string value - see packages/rest/src/coercion/coerce-parameter.ts
  4. Update our example Todo controller to use @param.query.jsonObject - see examples/todo/src/controllers/todo.controller.ts
  5. Start the Todo app, open API Explorer and check how is the filter parameter handled.

Acceptance Criteria

- [ ] Review the workaround posted by various users in #2208

  • Try out the experiment from @bajtos (listed above)
@bajtos
Copy link
Member

bajtos commented Sep 20, 2019

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?

@deepakrkris
Copy link
Contributor

deepakrkris commented Sep 23, 2019

For having the "include" parameter in filter condition, filter["include"]= does not work whereas passing filter as deep object works.
example:
http://localhost:3000/todos/1?filter=%7B%22include%22%3A%5B%7B%22relation%22%3A%22todoList%22%7D%5D%7D

@deepakrkris
Copy link
Contributor

url encoding of json can be done with javascript built in function encodeURIComponent(stringified-json)

@deepakrkris
Copy link
Contributor

deepakrkris commented Nov 18, 2019

In the open api spec (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#encoding-object) , explanation for explode setting is,

When this is true, property values of type array or object generate 
separate parameters for each value of the array, or key-value-pair of the map.
For other types of properties this property has no effect.

@deepakrkris
Copy link
Contributor

spike #4141 tries to test passing complex object as uri encoded values in query params

@deepakrkris
Copy link
Contributor

deepakrkris commented Nov 19, 2019

@bajtos the spike #4141 works perfectly fine, API explorer supports url encoding of complex object in query params when implementing steps suggested by you.

Please ignore my previous comment.

cc: @dhmlau

@deepakrkris
Copy link
Contributor

deepakrkris commented Nov 19, 2019

tried the changes with the todoList example successfully:

Screen Shot 2019-11-18 at 9 39 21 PM

@bajtos
Copy link
Member

bajtos commented Nov 21, 2019

I though I have posted a comment, but it looks it got lost :(

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
Copy link
Member

bajtos commented Nov 21, 2019

I though I have posted a comment, but it looks it got lost :(

Ah, never mind, the discussion is happening here:
#4141 (comment)

Please ignore my comment above and continue the discussion in the pull request.

@deepakrkris
Copy link
Contributor

findings of spike:

  • this is a breaking fix, needs semver major release
  • change existing param.query.object decorator to support json objects in query params
  • including content field in param spec, helps to support both exploded filter conditions like filter[limit] = 0 and json objects
  • parameter schema cannot have content field along with schema, explode, type fields. swagger validation fails (check with https://editor.swagger.io/) if they appear together.
  • loopback currently does not support extracting parameter schema from the content field. if we are including schema under content and not schema field, then it means additional changes in tests and source.
  • add additional unit, acceptance and regression tests

@deepakrkris
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users spike
Projects
None yet
Development

No branches or pull requests

3 participants