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: predicate logic against bools #10679

Merged
merged 4 commits into from Nov 16, 2022

Conversation

svidgen
Copy link
Contributor

@svidgen svidgen commented Nov 16, 2022

Description of changes

Predicate filtering logic (used by observe and observeQuery) was not working correctly against bools, because we were casting field values to strings before comparison. This appears to be cruft from POC work — I can't otherwise tell why this cast would have been in place.

Issue #, if available

Description of how you validated changes

Added tests that were red. Made the change. Tests turned green.

Checklist

  • PR description included
  • yarn test passes
  • Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@svidgen svidgen requested a review from a team as a code owner November 16, 2022 03:59
@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #10679 (1254170) into main (056259d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #10679      +/-   ##
==========================================
+ Coverage   85.60%   85.62%   +0.01%     
==========================================
  Files         196      196              
  Lines       18241    18243       +2     
  Branches     3887     3887              
==========================================
+ Hits        15616    15620       +4     
+ Misses       2547     2546       -1     
+ Partials       78       77       -1     
Impacted Files Coverage Δ
packages/datastore/__tests__/model.ts 100.00% <ø> (ø)
packages/datastore/__tests__/schema.ts 100.00% <ø> (ø)
packages/datastore/__tests__/helpers.ts 84.06% <100.00%> (+0.03%) ⬆️
packages/datastore/src/predicates/next.ts 94.67% <100.00%> (+0.02%) ⬆️
packages/datastore/src/types.ts 86.47% <0.00%> (+1.17%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

iartemiev
iartemiev previously approved these changes Nov 16, 2022
Copy link
Contributor

@iartemiev iartemiev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍🏻

Copy link
Contributor

@dpilch dpilch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add tests for eq and ne on number fields. I found that casting to string also affects numbers. https://github.com/dpilch/predicate-tests

dpilch
dpilch previously approved these changes Nov 16, 2022
iartemiev
iartemiev previously approved these changes Nov 16, 2022
packages/datastore/__tests__/DataStore.ts Outdated Show resolved Hide resolved
packages/datastore/__tests__/DataStore.ts Show resolved Hide resolved
packages/datastore/__tests__/DataStore.ts Show resolved Hide resolved
packages/datastore/__tests__/DataStore.ts Show resolved Hide resolved
packages/datastore/__tests__/DataStore.ts Outdated Show resolved Hide resolved
@svidgen svidgen dismissed stale reviews from iartemiev and dpilch via a0685bd November 16, 2022 16:43
@svidgen svidgen merged commit 062cb55 into aws-amplify:main Nov 16, 2022
@svidgen svidgen deleted the fix-boolean-matching branch November 16, 2022 17:00
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.

None yet

5 participants