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

fix(knex): Fix Knex adapter date comparison queries #3429

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

doug-patterson
Copy link
Contributor

Summary

  • Currently the schema-driven development system claims that Type.Date is acceptable, but date comparison queries are silently ignored, e.g. app.service('foo').find({ query: { closeTime: { $gt: new Date() } } }) just gets you all results.
  • I have been unable to find a matching issue
  • No dependencies on other PRs

Other Information

The issue here is that a date is an object, so the code in knex/adapter tries to get the query operators as its keys and of course finds none so it just moves on. The fix here just lets a date be a date in comparisons, too.

Problem I of course want to include some unit tests for this. However, the typing in the tests somehow has been set up with Omit called on the enum for acceptable JSON Schema types - you can't make a json schema with a property that has { type: 'date' } to feed into the the query syntax, so it's impossible to construct a proper test involving dates. I'll 'fess up and admit that I tried to figure this out for several hours on Monday but couldn't.

If someone who knows the codebase better than I do can point me to where { type: 'date' } is ruled out by the typing in there I'll get this finished. Apologies for not finding it myself but that's how it is and I'm hoping someone with more knowledge than me can help me with it.

Thanks! And thanks also to @daffl and all contributors for making the best of all API frameworks in any language!

@daffl daffl changed the title Fix Knex adapter date comparison queries fix(kned): Fix Knex adapter date comparison queries Feb 15, 2024
@daffl daffl changed the title fix(kned): Fix Knex adapter date comparison queries fix(knex): Fix Knex adapter date comparison queries Feb 15, 2024
@daffl daffl merged commit 23bafe1 into feathersjs:dove Feb 15, 2024
2 checks passed
@daffl
Copy link
Member

daffl commented Feb 15, 2024

Thank you for the pull request. This should be fine since the change seems pretty safe and all tests are still passing. Released in v5.0.22

@doug-patterson
Copy link
Contributor Author

Thanks @daffl - I'll hope to contribute again soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants