REST API to find models using a filter #1679

bajtos · 2018-09-07T13:40:00Z

This pull request leverages the recently added @param.query.object() decorator (see #1667) to add support for querying a subset of models using a Filter object.

For example:

GET /todos?filter[where][isCompleted]=false

The OpenAPI spec contains a reasonably-minimal description of the Filter object.

Done

infrastructure to describe filter and where parameters
updated todo example
update todo-list example
update CRUD Controller template in CLI
doc updates

Checklist

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

bajtos · 2018-09-07T14:05:04Z

Regarding API Explorer/swagger-ui - I am not able to send a request with a filter value, it looks like the UI is somehow not passing the filter through. I'll have to investigate. According to swagger-api/swagger-ui#4216, style: deepObject should be working.

b-admike · 2018-09-07T14:23:15Z

packages/repository-json-schema/test/unit/filter-json-schema.unit.ts

+import {JsonSchema} from '../../src';
+
+describe('getFilterJsonSchemaFor', () => {
+  @model()


Can we define models at the bottom of the test file?

b-admike · 2018-09-07T14:29:09Z

packages/repository-json-schema/test/unit/filter-json-schema.unit.ts

+  it('describes "fields" as an object', () => {
+    const filter = {fields: 'invalid-fields'};
+    ajv.validate(customerFilterSchema, filter);
+    expect(ajv.errors || []).to.containDeep([


I'm curious; why are we doing ajv.errors || [] here?

When ajv.validate returns true, ajv.errors is set to null. As a result, shouldjs produced error message that seemed to me as more difficult to understand.

I am open to suggestions on how to improve this assertion and/or make the code easier to understand!

raymondfeng · 2018-09-07T17:58:56Z

Regarding API Explorer/swagger-ui - I am not able to send a request with a filter value

What about stringified json? For qs style such as filter[where][isCompleted]=false, I don't know if possible to type into the filter text in the form.

bajtos · 2018-09-10T10:47:01Z

Regarding API Explorer/swagger-ui - I am not able to send a request with a filter value

I did some more investigation. It seems that we need to upgrade our api-explorer with the latest swagger-ui version to bring in deepObject fix from swagger-api/swagger-js#1235. I opened a pull request for that - see loopbackio/loopback.io#736

We should also set explode: true in a deep-object parameter definition, I'll fix that as part of this pull request.

With those changes in place, I am able to send a request like ?filter[where]=true by entering {"where": true} to the input box:

Unfortunately, when I provide a value with nested properties, e.g. {"where":{"id: 1}}, swagger-ui ignores such parameter value :(

Based on the following discussions, I think it may be up to us to contribute serialization of deeply nested objects:

swagger-api/swagger-js#1140

Limitations:
deepObject does not handle nested objects. The specification and swagger.io documentation does not provide an example for serializing deep objects. Flat objects will be serialized into the deepObject style just fine.

swagger-api/swagger-js#1140 (comment)

As for deepObject and nested objects - that was explicitly left out of the spec, and it's ok to just Not Support It™.

Related issues: swagger-api/swagger-ui#4216 and swagger-api/swagger-ui#4064

What about stringified json? For qs style such as filter[where][isCompleted]=false, I don't know if possible to type into the filter text in the form.

Unfortunately not. Because the OAS says filter is an object, the user has to enter valid JSON.

When I tried to enter a string containing the JSON, e.g. "{\"where\":{\"id\":1}}, it triggered a validation error in swagger-ui's user interface.

bajtos · 2018-09-10T11:06:09Z

I left a comment in a swagger-ui issue to discuss this problem further, see swagger-api/swagger-ui#4064 (comment)

@raymondfeng @strongloop/sq-lb-apex what's your opinion on the next steps in this pull request? Should I continue adding filter parameter described as {type: 'object', style: 'deepObject', explode: true}, despite the fact that swagger-ui does not support such filter values at the moment? Should we put this pull request on hold until swagger-ui supports our parameter style?

Here is my concern with landing these changes: users trying out LoopBack 4 and the API Explorer will quickly discover that they cannot send filter values. Because swagger-ui does not provide any hints on what's wrong, I expect a lot of confusion among our users, which may cast bad light on our new major version.

Thoughts?

UPDATE 2018-09-13 I opened a new issue in swagger-js with the hope that it will get more attention. See swagger-api/swagger-js#1385

virkt25 · 2018-09-10T18:08:40Z

It will definitely be an annoyance of not being able to test a filter from the explorer because it doesn't support deepObject serialization. I think the next steps should be to put this PR on hold ... possibly contribute a patch to swagger-ui and then continue work on this PR. Without support for this in swagger-ui this change to our code doesn't necessarily help users looking to test changes via the explorer.

jannyHou

~~LGTM! 🚢~~
Wait I open the PR a few days ago and didn't see the new discussion regarding swagger-ui.
The code change in this PR LGTM but if it causes the explorer ignoring the filter then let's fix the swagger-ui first.

jannyHou · 2018-09-10T19:01:57Z

packages/repository-json-schema/src/filter-json-schema.ts

+          // TODO(bajtos) restrict values to relations defined by "model"
+          relation: {type: 'string'},
+          // TODO(bajtos) describe the filter for the relation target model
+          scope: scopeFilter,


Not involved in this PR:
I think we can use a $ref here and append the related model's filter schema in spec.components.schemas when work on describing the filter for the related model.

jannyHou · 2018-09-10T19:16:38Z

examples/todo/test/acceptance/todo.acceptance.ts

+  // result returned by REST API does not.
+  // Use this function to convert a model instance into a data object
+  // as returned by REST API
+  function deserializedFromJson<T extends object>(value: T): T {


nitpick: not a necessary part of this PR, we can update this test to call deserializedFromJson as well :)

jannyHou

~~LGTM! 🚢~~
Wait I open the PR a few days ago and didn't see the new discussion regarding swagger-ui.
The code change in this PR LGTM but if it causes the explorer ignoring the filter then let's fix the swagger-ui first.

raymondfeng · 2018-09-11T20:50:26Z

Maybe we should start with what LB3 allows - when the openapi spec is generated, we mark filter as a string with x-format or x-style as json.

virkt25

Excellent PR! Thinking more about my previous comment, I think it's probably better to not wait for swagger-ui to be fixed and this PR should be landed as most users creating apps will not be consuming the APIs using the explorer (its great for testing) but not real life usage. For real life usage I think it's best to land the ability to filter a Model ASAP. :)

virkt25 · 2018-09-12T00:16:03Z

packages/repository-json-schema/src/filter-json-schema.ts

+
+  // TODO(bajtos) Rework this condition to check whether "model has any
+  // relations defined
+  if (modelCtor !== EmptyModel) {


When do we expect a modelCtor to be an EmptyModel. Not sure I follow this logic.

When a model has relations configured, the filter can provide include property to describe which related models to include in the result. Think about object-tree traversal in GraphQL.

So for example, when a Customer has many Order instances, and Order instance belongs to a Customer, it's possible to craft the following Customer query:

{ where: { email: 'mbajtoss@gmail.com' }, include: [{ relation: 'orders', scope: { // a full Filter for Order model where: {totalPrice: {gte: 100}}, // we can do recursion too! include: [{ relation: 'customer', }], }, }

Ideally, we would like to describe include[].filter property as a Filter schema tailored to the properties and relations available at the target model of the relation. I decided to leave that out of the initial implementation and describe filter as a Filter object for a model with no pre-defined properties but allowing any extra properties:

const scopeFilter = getFilterJsonSchemaFor(EmptyObject);

However, this creates a recursion - in order to build schema for scopeFilter, I need to describe include[].filter property as scopeFilter too.

The check modelCtor !== EmptyModel is just a quick way how to detect this situation, avoid infinite recursion and also leave out the property include from the nested include[].filter object (for better or worse).

My takeaway is that I should fix this condition to check the actual relations defined on the model, and do that before this pull request is landed.

bajtos

The patch is ready for final review and landing.

@raymondfeng @virkt25 @jannyHou @b-admike PTAL

bajtos · 2018-09-20T12:27:58Z

packages/testlab/src/index.ts

@@ -12,3 +12,4 @@ export * from './test-sandbox';
 export * from './skip-travis';
 export * from './request';
 export * from './http-server-config';
+export * from './misc';


This is from #1733, git should remove it after #1733 lands and this patch is rebased on top of the new master.

bajtos · 2018-09-20T12:28:04Z

packages/testlab/src/misc.ts

+ * Use this function to convert a model instance into a data object
+ * as returned by REST API
+ */
+export function deserializedFromJson<T extends object>(value: T): T {


Ditto: This is from #1733, git should remove it after #1733 lands and this patch is rebased on top of the new master.

virkt25

Looks great! Excited to see functional filter support. Left some comments but overall looks good.

virkt25 · 2018-09-24T03:31:22Z

packages/cli/generators/controller/templates/src/controllers/controller-rest-template.ts.ejs

@@ -68,10 +68,10 @@ export class <%= className %>Controller {
    },
  })
  async updateAll(
-    @requestBody() <%= name %>: <%= modelName %>,
+    @requestBody() data: <%= modelName %>,


Not a big deal but I personally prefer not using a generic name like data since it would repeat multiple times and as such would much rather have it be the model name ... todo: Todo.

Makes sense, will go with todo: Todo.

Before my change, we were using the controller name as the argument name, e.g. TodoController: Todo.

virkt25 · 2018-09-24T03:39:47Z

packages/repository-json-schema/src/filter-json-schema.ts

+    properties: {
+      where: {
+        type: 'object',
+        // TODO(bajtos) enumberate "model" properties and operators like "and"


WIll this be a part of this PR?

I prefer to leave it out, I feel I am already beyond the scope described in #100.

Here is the follow-up issue I created, I'll mention it in the code comment too:
#1748

virkt25 · 2018-09-24T03:44:19Z

docs/site/Controller-generator.md

+        content: {'application/json': {'x-ts-type': Number}},
+      },
+    },
+  })
  async count(@param.query.string('where') where?: Where): Promise<number> {


Shouldn't this be changed to @param.query.object('where')?

Good point!

virkt25 · 2018-09-24T03:44:58Z

docs/site/todo-list-tutorial-controller.md

+      },
+    },
+  })
+  async create(@requestBody() obj: TodoList): Promise<TodoList> {


obj => data as per the new CLI template.

virkt25 · 2018-09-24T03:45:41Z

docs/site/todo-list-tutorial-controller.md

  async find(
-    @param.path.number('id') id: number,
    @param.query.string('filter') filter?: Filter,


This should be @param.query.object

docs/site/Controller-generator.md

virkt25 · 2018-09-24T03:53:46Z

packages/repository-json-schema/src/filter-json-schema.ts

+
+      fields: {
+        type: 'object',
+        // TODO(bajtos) enumberate "model" properties


Will this be a part of this PR?

No, see #1748

jannyHou · 2018-09-24T04:08:26Z

docs/site/Controller-generator.md

  async updateAll(
-    @requestBody() obj: Todo,
+    @requestBody() data: Todo,
    @param.query.string('where') where?: Where,


shouldn't where an object instead of string?

shouldn't where an object instead of string?

Good point, will fix.

jannyHou

@bajtos Great effort!
I left a few nitpick for updating the doc.
And one design question regarding the relation metadata (don't think it's this in PR's scope to address it, we can discuss in a new issue if you also think my concern is valid, thanks!)

jannyHou · 2018-09-24T04:11:17Z

docs/site/Controller-generator.md

+        content: {'application/json': {'x-ts-type': Number}},
+      },
+    },
+  })
  async count(@param.query.string('where') where?: Where): Promise<number> {


same here @param.query.object('where')

jannyHou · 2018-09-24T04:14:29Z

docs/site/todo-list-tutorial-controller.md

+      },
+    },
+  })
+  async count(@param.query.string('where') where?: Where): Promise<number> {


@param.query.object('where')

jannyHou · 2018-09-24T04:41:11Z

packages/repository/test/unit/decorator/relation.decorator.unit.ts

+    const relations = getModelRelations(Customer);
+    expect(relations).to.deepEqual({
+      accessTokens: {
+        keyTo: 'userId',


wouldn't it be customerId?
Otherwise when we have multiple models extend User, they will share the same fk which is userId...

No, it's userId because Customer inherited the relation the User class.

In LB 3.x, we call this a "polymorphic relation" and add another property to accessToken to distinguish between different targets that can have the same userId. For example, accessToken can have two properties: userId for the id (PK/FK) value, userModel for a name of the target model class (User, Customer, etc.)

The purpose of the test is just to verify that the new function returns all relations - defined on the model and inherited from parents.

we call this a "polymorphic relation"

oh ok I got it 👍 it's polymorphic.

bajtos · 2018-09-24T08:54:55Z

@jannyHou @virkt25 thank you for a thorough review, you spotted important places to fix!

In order to support where parameters, I added a new helper getWhereSchemaFor.

Then I used this new helper in all controller-related code and docs, plus addressed your other comments too.

Could you please take another look and let me know if the changes LGTY now?

bajtos · 2018-09-24T08:56:02Z

I have also created two follow-up stories (post 4.0 GA):

Enumerate model properties in Filter/Where schema Enumerate model properties in Filter/Where schema #1748
Sugar decorators for Filter and Where parameters Sugar decorators for Filter and Where parameters #1749

virkt25 · 2018-09-24T15:33:16Z

packages/openapi-v3/src/filter-schema.ts

+ *
+ * @param modelCtor The model constructor to build the filter schema for.
+ */
+export function getWhereSchemaFor(modelCtor: typeof Model): SchemaObject {


Since we include getWhereSchemaFor in this file, would it rename this file to get-partial-schema? or something along those lines?

This file contains functions to build schema of Filter and Where objects. I feel get-partial-schema is misleading, because both Filter and Where are much more than just an object allowing a subset of model properties. For example, Where can contain conditions like and and custom operators like neq, gte, etc.

As I see it, both Filter and Where are related to querying/filtering model instances. Where describes condition(s) to apply, Filter provides additional query instructions like pagination and which fields & relations to fetch.

That's why I think filter-schema is a good file name.

virkt25 · 2018-09-24T15:34:01Z

packages/repository-json-schema/src/filter-json-schema.ts

+ *
+ * @param modelCtor The model constructor to build the filter schema for.
+ */
+export function getWhereJsonSchemaFor(modelCtor: typeof Model): JsonSchema {


ditto: Should we consider renaming the file?

virkt25

LGTM. A non-blocker comment on file names.

Implement new APIs for building JSON and OpenAPI schemas describing the "filter" and "where" objects used to query or modify model instances.

Modify the CLI template and example controllers to accept a new filter argument, leveraging the recently added `@param.query.object()` and Filter schema. Rename the controller-method argument accepting model data from `${controllerName}` to `data` (e.g. `data` instead of `TodoController`). I think this was an oversight in the implementation of the REST Controller template. Update the code snippets in documentation that are showing controller classes to match the actual code used in the examples and produced by our `lb4 controller` command.

hadasiddhant · 2019-05-05T21:11:39Z

It would be of great help if anyone here can answer this question for me. Thanks a lot
https://stackoverflow.com/questions/55996451/how-do-i-query-a-particular-field-in-loopback-4-through-the-repository

bajtos added this to the September Milestone milestone Sep 7, 2018

bajtos self-assigned this Sep 7, 2018

bajtos requested review from raymondfeng, virkt25, jannyHou and b-admike September 7, 2018 13:40

bajtos added the REST Issues related to @loopback/rest package and REST transport in general label Sep 7, 2018

bajtos changed the title ~~WIP: a REST API to find models using a filter~~ [WIP] a REST API to find models using a filter Sep 7, 2018

b-admike reviewed Sep 7, 2018

View reviewed changes

bajtos mentioned this pull request Sep 10, 2018

Sending an object list in the URL swagger-api/swagger-ui#4064

Closed

bajtos force-pushed the feat/find-with-filter branch from a9779c2 to bc9e2f6 Compare September 10, 2018 12:00

jannyHou approved these changes Sep 10, 2018

View reviewed changes

jannyHou reviewed Sep 10, 2018

View reviewed changes

virkt25 reviewed Sep 12, 2018

View reviewed changes

bajtos added the blocked label Sep 13, 2018

bajtos mentioned this pull request Sep 13, 2018

Support deep objects for query parameters with deepObject style swagger-api/swagger-js#1385

Open

raymondfeng mentioned this pull request Sep 17, 2018

user order history component loopbackio/loopback4-example-shopping#12

Merged

bajtos mentioned this pull request Sep 18, 2018

Add support for "include" and "fields" to findById (REST API) #1721

Closed

6 tasks

bajtos force-pushed the feat/find-with-filter branch 2 times, most recently from b4bba3d to c4b08d7 Compare September 20, 2018 12:26

bajtos changed the title ~~[WIP] a REST API to find models using a filter~~ REST API to find models using a filter Sep 20, 2018

bajtos commented Sep 20, 2018

View reviewed changes

bajtos requested review from virkt25 and b-admike September 20, 2018 12:28

bajtos requested a review from jannyHou September 20, 2018 12:28

bajtos removed the blocked label Sep 21, 2018

virkt25 reviewed Sep 24, 2018

View reviewed changes

jannyHou reviewed Sep 24, 2018

View reviewed changes

jannyHou approved these changes Sep 24, 2018

View reviewed changes

This was referenced Sep 24, 2018

Enumerate model properties in Filter/Where schema #1748

Closed

Sugar decorators for Filter and Where parameters #1749

Closed

bajtos force-pushed the feat/find-with-filter branch 2 times, most recently from 5ce9332 to a4b7198 Compare September 24, 2018 08:52

bajtos mentioned this pull request Sep 24, 2018

fix(rest): coerce string parameters (reject object values) #1750

Merged

3 tasks

virkt25 reviewed Sep 24, 2018

View reviewed changes

virkt25 approved these changes Sep 24, 2018

View reviewed changes

bajtos added 2 commits September 25, 2018 10:27

feat: builders for Filter and Where schemas

1645d56

Implement new APIs for building JSON and OpenAPI schemas describing the "filter" and "where" objects used to query or modify model instances.

bajtos force-pushed the feat/find-with-filter branch from a4b7198 to 6e9d2a9 Compare September 25, 2018 08:30

bajtos merged commit 7e1acfc into master Sep 25, 2018

bajtos deleted the feat/find-with-filter branch September 25, 2018 08:45

This was referenced Oct 11, 2018

Support deep objects for query parameters with deepObject style OAI/OpenAPI-Specification#1706

Open

[API Explorer] Fields shown but ignored #1470

Closed

bajtos mentioned this pull request Jan 10, 2019

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

Closed

7 tasks

bajtos mentioned this pull request Apr 29, 2019

API Explorer ignores where filter #2804

Closed

loopbackio locked as resolved and limited conversation to collaborators May 9, 2019

REST API to find models using a filter #1679

REST API to find models using a filter #1679

Conversation

bajtos commented Sep 7, 2018 • edited Loading

Checklist

bajtos commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Sep 7, 2018

bajtos commented Sep 10, 2018 • edited Loading

bajtos commented Sep 10, 2018 • edited Loading

virkt25 commented Sep 10, 2018

jannyHou left a comment • edited Loading

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

raymondfeng commented Sep 11, 2018

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

virkt25 left a 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

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

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

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 Sep 24, 2018 • edited Loading

Choose a reason for hiding this comment

bajtos commented Sep 24, 2018

bajtos commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

virkt25 left a comment

Choose a reason for hiding this comment

hadasiddhant commented May 5, 2019

bajtos commented Sep 7, 2018 •

edited

Loading

bajtos commented Sep 10, 2018 •

edited

Loading

bajtos commented Sep 10, 2018 •

edited

Loading

jannyHou left a comment •

edited

Loading

jannyHou Sep 24, 2018 •

edited

Loading