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

Querying date using Rest API where clause #3543

Closed
Aure77 opened this issue Feb 3, 2016 · 7 comments
Closed

Querying date using Rest API where clause #3543

Aure77 opened this issue Feb 3, 2016 · 7 comments
Labels
blueprints Issue only occurs when using the blueprint API needs failing test orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. try this out please

Comments

@Aure77
Copy link

Aure77 commented Feb 3, 2016

If I send this where clause in a rest api call :
{'updatedAt': {'>':'2016-01-01T00:00:00.000Z'}
(updatedAt greater than 2016-01-01)

With this url for example :
http://localhost:1337/api/rest/tournament?where={%27updatedAt%27:{%27%3E%27:%272016-02-02T10:57:08.976Z%27}}

This will output data without filtering by updatedAt :

[
  {
    "name": "Test1",
    "createdAt": "2016-02-01T21:00:14.925Z",
    "updatedAt": "2016-02-02T22:28:58.793Z",
    "id": 1
  },
  {
    "name": "Test2",
    "createdAt": "2015-12-03T10:57:08.992Z",
    "updatedAt": "2015-12-03T10:57:08.992Z",
    "id": 2
  }
]

Test2 should not be returned.

What is the correct way to querying date using where clause (in the rest api) ?

@mikermcneil
Copy link
Member

Hi @Aure77 - would you mind adding information about which adapter and Sails version you're using? Thanks!

@Aure77
Copy link
Author

Aure77 commented Feb 3, 2016

Sails version : 0.12.0-rc6
Db Adapter : [email protected]

@mikermcneil
Copy link
Member

@Aure77 So @particlebanana did some work tracking this down, and here's the situation: In this particular case, the fix is in waterline-criteria (a dependency of sails-disk). Will keep you posted on that (we'll do a patch for 0.11.x). Thanks again!

I also wanted to take some time to provide background on this for your future reference, and for anyone else reading this who wants to learn more about how Waterline is architected or interested in contributing. tldr; There are a few different longer-term roadmap items we're working on that will help make issues like these easier to prevent cross-adapter in the future.

The reason the fix is in waterline-criteria is because waterline-criteria is a dependency used by sails-disk (it is also used in waterline core, but that's just coincidence). Currently, adapters are responsible for coercing incoming criteria themselves. This is a decision we made initially so that individual adapters could apply their own understanding of data types. This works fairly well, but it does mean that behavior can be inconsistent across adapters (e.g. if you were to try the exact same thing using sails-postgresql, it should work).

We'll always need to apprehend issues like these on a case by case basis (as I mentioned above, keep an eye out for a patch that will address this for apps using either 0.11.x or 0.12.x.) But longer term, we have some plans for Sails v1.0 that will help make this easier to resolve by providing a deeper separation between:

  • (A) the code which interprets usage in your app and figures out how best to query models (Waterline core)
  • (B) the code which compiles criteria dictionaries into e.g. SQL or Mongo queries and communicates with the underlying database (adapters), and
  • (C) the code that builds the physical-layer schema in the first place; i.e. automigrations (currently part of Waterline core, planning to pull that out into Sails core for v1)

This will allow us to do more in Waterline core as far as type validation/coercion, since the way data type of attributes is expressed will more clearly specify its logical type (e.g. "string") vs. its physical type (e.g. "VARCHAR(255)").

However, relevant to this specific case, "date" will likely never be treated as a top-level logical type. It's hard to say for sure of course-- but we've learned some pertinent lessons in that department when building rttc, and at this point my hunch is that we should continue to have dates come in as either UTC (timezone-agnostic) ISO strings like '2016-02-05T10:51:34.299Z') or js (epoch ms) timestamps (e.g. 1454669466490). Of course, to do this, that means that these values will need to continue to be interpreted when building physical queries in the adapter.

Which brings up another consideration which was also brought up recently by the guys at Postman: Adapters need standardized access to the logical schema (e.g. "string" or "json") and the physical schema (e.g. "datetime" or "geojson"). The latter can vary across adapters, and may or may not even be specced in your model today. Currently, adapters grab hold of this information initially, then maintain private state. But a better approach would be for Waterline core to always coerce incoming criteria and values to match the logical schema, and that's the plan.

In addition, Waterline core could provide a (preferably immutable) reference to the physical schema obtained by an initial call to the adapter's .describe() method. This would allow Waterline to flush itself as the schema changes or connection info changes, and prevent adapters from having to maintain their own state (assuming a similar solution was implemented for the connection pool obtained from the underlying driver-- which is something we've begun work on, since it is necessary for easier-to-use native transaction support).

So to wrap that up, here's a quick recap:

  • Waterline adapters will always be responsible for mapping incoming logical criteria to the relevant physical type in the database. This is necessary to empower adapter authors with enough flexibility to take advantage of advanced database-specific features.
  • However, adapters shouldn't have to be responsible for ensuring that criteria and data provided at runtime match the expected logical type. In other words, to stick to your example, an adapter will still be responsible for recognizing that your updatedAt column is a "DATETIME" and that an incoming '2016-01-01T00:00:00.000Z' in {'updatedAt': {'>':'2016-01-01T00:00:00.000Z'} } needs to be understood and compiled as a datetime comparison, rather than as a string comparison. But, since we know logically that updatedAt is supposed to be a string, if something like {'updatedAt': {'>': new Readable() } } is provided, it'll be rejected before it ever makes it down to the adapter.

@mikermcneil
Copy link
Member

@Aure77 just published a pre-release patch of sails-disk-- would you try it out when you have a sec?

# in the root folder of your Sails app:
npm install sails-disk@beta --force

Thanks!

@mikermcneil mikermcneil added blueprints Issue only occurs when using the blueprint API orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. labels Feb 5, 2016
@Aure77
Copy link
Author

Aure77 commented Feb 8, 2016

After updating sails-disk, I am always experiencing same issue.
But I found a solution by changing quote type (' -> ") :
{"updatedAt": {">":"2016-01-01T00:00:00.000Z"}

About the related pull request, where.js seems to already handle date comparison ? :
https://github.com/balderdashy/waterline-criteria/blob/1f6eccfa571fb5121fabdacb0bfb474335432117/lib/filters/where.js#L193

@mikermcneil
Copy link
Member

@Aure77 correct, using query params in the blueprint API, you'll always need to use JSON encoding (double quotes) My bad, should have caught that earlier, but couldn't tell from your original post because of the URL encoding. Regardless, I don't believe this should have worked prior to the update because sails-disk wasn't passing in the expected schema to wl-criteria (unless i'm missing something)

@sgress454
Copy link
Member

0.12.1 has been released which points to the updated version of Waterline and its dependencies. Tested and confirmed that this is fixed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blueprints Issue only occurs when using the blueprint API needs failing test orm Related to models, datastores, orm config, Waterline, sails-hook-orm, etc. try this out please
Development

No branches or pull requests

3 participants