Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.3k
fix(range-bounds): observe inclusive range bounds#9364
fix(range-bounds): observe inclusive range bounds#9364sushantdhiman merged 3 commits intosequelize:masterfrom
Conversation
codecovbot commentedApr 27, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
9f9ec8f toa6e10abComparelib/utils.js Outdated
| obj = obj || {}; | ||
| return _.cloneDeepWith(obj, elem => { | ||
| // Special handling for the inclusive array property (for range types) | ||
| if (Array.isArray(elem) && elem.hasOwnProperty('inclusive')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will execute for each clone operation I would like to avoid this if possible, there is also one more issue#8471
As you are using this type in real world, what do you think about having range type store values like
[{value:10,inclusive:true},{value:10,inclusive:true}]
Rather than having non-standard array property?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree that not having to worry about special handling for the non-standard "inclusive" array property would be much cleaner. We should move to a standard representation of range types for both query predicates and query results ensuring the inclusive property is still maintained when results are converted to JSON.
This will be a breaking change though, as we (and I expect many others) have a lot of code that expects range values returned in query results to be of the form
[ 10, 10 ]Another approach might be to allow range types to have a 3rd "options" element in the array. When querying/inserting you could define your range as any of
[ 10, 10 ] // Defaults to inclusive [true, false][ 10, 10, { inclusive: true }][ 10, 10, { inclusive: [ true, false ] }]Query results would always be returned as
[ 10, 10, { inclusive: [ true, false ] }]This will still be a breaking change, but may lessen the impact on those queries that currently use ranges in their default form (without being concerned about inclusivity).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No problem. Is it ok if I make these changes under this pull request? There'll be some documentation changes too which I'll try to cover.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah no problem
sushantdhiman left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good, just a small documentation change
| range // [{ value: Date, inclusive: false }, { value: Date, inclusive: true }] | ||
| ``` | ||
| Make sure you turn that into a serializable format before serialization since array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A note here saying that you will need to callreload after updating an instance with range type or usereturning: true option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
BREAKING CHANGE: range values are now returned as an array of two objects with each containing a property for the value and a property for its bounds inclusion, rather than bounds inclusion being defined as an array property. When inserting/querying, bounds inclusion must also be defined in the same way. This change ensures that bounds inclusivity is not lost when serializing or logging data returned in query results. To migrate code, follow the example below: Before: range: [10, 20]; range.inclusive = [false, true]; After: range: [{ value: 10, inclusive: false }, { value: 20, inclusive: true }];Closessequelize#8176,sequelize#8471
Uh oh!
There was an error while loading.Please reload this page.
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Description of change
Ensure the 'inclusive' property on range bounds is not lost when querying.
Closes#8176,#8471