fix(range-bounds): observe inclusive range bounds #9364
Conversation
9f9ec8f to
a6e10ab
Compare
lib/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.
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.
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.
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.
sushantdhiman
left a comment
There was a problem hiding this comment.
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.
A note here saying that you will need to call reload after updating an instance with range type or use returning: true option
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 }];
Closes sequelize#8176, sequelize#8471
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