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

Range bounds are not applied #8176

Closed
skilFullGH opened this issue Aug 23, 2017 · 15 comments
Closed

Range bounds are not applied #8176

skilFullGH opened this issue Aug 23, 2017 · 15 comments

Comments

@skilFullGH
Copy link

@skilFullGH skilFullGH commented Aug 23, 2017

What you are doing?

I compare time range with inclusive bounds.

let range = [
      data.period[0],
      data.period[1]
    ]
    range.inclusive = true

const d = await model.findOne({
      where: {
        id: {
          $ne: data.id
        },
        StudentId: data.Student.id,
        TimePeriodId: data.TimePeriod.id,
        WeekDayId: data.WeekDay.id,
        level,
        period: {
          $overlap: range
        }
      }
    })

What do you expect to happen?

Query

SELECT "id", "period", "level", "studyYear", "order" FROM "AsSchedules" AS "AsSchedule" WHERE "AsSchedule"."id" != 0 AND "AsSchedule"."StudentId" = 507 AND "AsSchedule"."TimePeriodId" = 8 AND "AsSchedule"."WeekDayId" = 1 AND "AsSchedule"."level" = 1 AND "AsSchedule"."period" && '["2016-11-04 21:00:00.000 +00:00","2016-11-05 21:00:00.000 +00:00"]' LIMIT 1

What is actually happening?

But receive

SELECT "id", "period", "level", "studyYear", "order" FROM "AsSchedules" AS "AsSchedule" WHERE "AsSchedule"."id" != 0 AND "AsSchedule"."StudentId" = 507 AND "AsSchedule"."TimePeriodId" = 8 AND "AsSchedule"."WeekDayId" = 1 AND "AsSchedule"."level" = 1 AND "AsSchedule"."period" && '["2016-11-04 21:00:00.000 +00:00","2016-11-05 21:00:00.000 +00:00")' LIMIT 1

Property inclusive lost after code

options = Utils.cloneDeep(options);

in findOne() function.

Dialect: postgres
__Database version: 9.6.3
__Sequelize version: 4.7.1

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Aug 29, 2017

May be fixed #8210

@skilFullGH
Copy link
Author

@skilFullGH skilFullGH commented Aug 31, 2017

Checked, the problem remained

@stale
Copy link

@stale stale bot commented Dec 9, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment 🙂

@stale stale bot added the stale label Dec 9, 2017
@stale stale bot closed this Dec 16, 2017
@mjy78
Copy link
Contributor

@mjy78 mjy78 commented Mar 15, 2018

I believe this problem still persists.

Database Version: postgres 9.6.8
Sequelize Version: 4.26.1

In my case I'm querying against a date range (tstzrange in Postgres). Regardless of how I set the inclusivity the query that is generated is always '[)'.

range.inclusive = true;, range.inclusive = [true, true]; and range = [ { value: fromDate, inclusive: true}, { value: toDate, inclusive: true }]; all result in a query that is '[)' instead of '[]'.

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Mar 15, 2018

Can you try with latest version

@mjy78
Copy link
Contributor

@mjy78 mjy78 commented Mar 15, 2018

Still the same result with 4.37.0 (I had a typo in the version above - I was actually running on 4.36.1).

@mjy78
Copy link
Contributor

@mjy78 mjy78 commented Mar 15, 2018

The problem is that _.cloneDeepWith() doesn't copy properties on an array object.

I can get it to work by adding special handling to the cloneDeep() function in 'lib/utils.js' as follows...

function cloneDeep(obj) {
  obj = obj || {};
  return _.cloneDeepWith(obj, elem => {

    // Special handling for the inclusive array property (for range types)
    if (Array.isArray(elem) && elem.hasOwnProperty('inclusive')) {
      var arr = _.map(elem, item => cloneDeep(item));
      arr.inclusive = cloneDeep(elem.inclusive);
      return arr;
    }

    // Do not try to customize cloning of arrays or POJOs
    if (Array.isArray(elem) || _.isPlainObject(elem)) {
      return undefined;
    }
   ...
   ...

This resolves the problem when using either range.inclusive = true; or range.inclusive = [true,true];.

Please note however that the documented alternative of using a single expression still doesn't work with this fix in place.

In the case where you define the range as range = [ { value: fromDate, inclusive: true}, { value: toDate, inclusive: true }]; the range is getting converted back to a simple [ fromDate, toDate ] somewhere by the time the stringify() function is called in 'lib/dialects/postgres/range.js'.

@sushantdhiman sushantdhiman reopened this Mar 15, 2018
@mjy78
Copy link
Contributor

@mjy78 mjy78 commented Mar 15, 2018

Further to this, I believe the problem with inclusivity being lost when using a single expression is in the RANGE.prototype._stringify() method in 'lib/dialects/postgres/data-types.js'. The map of the values in the range doesn't take this approach into account.

The modified version of the function below resolves that issue...

  RANGE.prototype._stringify = function _stringify(values, options) {
    if (!Array.isArray(values)) {
      return "'" + this.options.subtype.stringify(values, options) + "'::" +
        this.toCastType();
    }

    var valueInclusivity = [
      _.isArray(values.inclusive) ? values.inclusive[0] : values.inclusive === false ? false : true,
      _.isArray(values.inclusive) ? values.inclusive[1] : values.inclusive === true ? true : false
    ];
    const valuesStringified = values.map((value, index) => {
      if (value.hasOwnProperty('inclusive')) {
        valueInclusivity[index] = value.inclusive;
      }
      if (value.hasOwnProperty('value')) {
        value = value.value;
      }
      if (_.includes([null, -Infinity, Infinity], value)) {
        // Pass through "unbounded" bounds unchanged
        return value;
      } else if (this.options.subtype.stringify) {
        return this.options.subtype.stringify(value, options);
      } else {
        return options.escape(value);
      }
    });

    // Array.map does not preserve extra array properties
    valuesStringified.inclusive = valueInclusivity;

    return  '\'' + range.stringify(valuesStringified) + '\'';
  };

mjy78 pushed a commit to mjy78/sequelize that referenced this issue May 7, 2018
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
mjy78 pushed a commit to mjy78/sequelize that referenced this issue May 7, 2018
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
@theRichu
Copy link

@theRichu theRichu commented Sep 14, 2018

I think this is should be re-open

Tutorial says,

// defaults to '["2016-01-01 00:00:00+00:00", "2016-02-01 00:00:00+00:00")'
// inclusive lower bound, exclusive upper bound
Timeline.create({ range: [new Date(Date.UTC(2016, 0, 1)), new Date(Date.UTC(2016, 1, 1))] });

// control inclusion
const range = [new Date(Date.UTC(2016, 0, 1)), new Date(Date.UTC(2016, 1, 1))];
range.inclusive = false; // '()'
range.inclusive = [false, true]; // '(]'
range.inclusive = true; // '[]'
range.inclusive = [true, false]; // '[)'

// or as a single expression
const range = [
  { value: new Date(Date.UTC(2016, 0, 1)), inclusive: false },
  { value: new Date(Date.UTC(2016, 1, 1)), inclusive: true },
];
// '("2016-01-01 00:00:00+00:00", "2016-02-01 00:00:00+00:00"]'

// composite form
const range = [
  { value: new Date(Date.UTC(2016, 0, 1)), inclusive: false },
  new Date(Date.UTC(2016, 1, 1)),
];
// '("2016-01-01 00:00:00+00:00", "2016-02-01 00:00:00+00:00")'

Timeline.create({ range });

but It is not working

const range = [
  { value: new Date(Date.UTC(2016, 0, 1)), inclusive: false },
  { value: new Date(Date.UTC(2016, 1, 1)), inclusive: true },
];

and this is my fix

Sequelize.DATE.prototype._applyTimezone = function _applyTimezone (date, options) {
  const toApplyDate = _.get(date, 'value', date)
  if (options.timezone) {
    if (momentTz.tz.zone(options.timezone)) {
      date = momentTz(toApplyDate).tz(options.timezone)
    } else {
      date = moment(toApplyDate).utcOffset(options.timezone)
    }
  } else {
    date = momentTz(toApplyDate)
  }

  return date
}
const range = require('sequelize/lib/dialects/postgres/range')

// console.log(Sequelize)
Sequelize.DataTypes.postgres.RANGE.prototype._stringify = function _stringify (values, options) {
  if (!Array.isArray(values)) {
    return "'" + this.options.subtype.stringify(values, options) + "'::" +
      this.toCastType()
  }
  const valuesStringified = values.map(value => {
    if (includes([null, -Infinity, Infinity], value)) {
      // Pass through "unbounded" bounds unchanged
      return value
    } else if (this.options.subtype.stringify) {
      // console.log(value)
      return this.options.subtype.stringify(value, options)
    } else {
      return options.escape(value)
    }
  })

  // Array.map does not preserve extra array properties
  valuesStringified.inclusive = _.get(values, 'inclusive', _.map(values, 'inclusive')) // values.inclusive

  return '\'' + range.stringify(valuesStringified) + '\''
}

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 14, 2018

@theRichu Which version?

@theRichu
Copy link

@theRichu theRichu commented Sep 14, 2018

 "sequelize": "^4.38.1"

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 14, 2018

This is available on v5-beta as this was a breaking change

@theRichu
Copy link

@theRichu theRichu commented Sep 14, 2018

Yeah,
I agree

 range: [{ value: 10, inclusive: false }, { value: 20, inclusive: true }];

this form is more clear

but my code not cause breaking change, and resolve the bug

Sequelize.DATE.prototype._applyTimezone = function _applyTimezone (date, options) {
  const toApplyDate = get(date, 'value', date)
  if (options.timezone) {
    if (momentTz.tz.zone(options.timezone)) {
      date = momentTz(toApplyDate).tz(options.timezone)
    } else {
      date = moment(toApplyDate).utcOffset(options.timezone)
    }
  } else {
    date = momentTz(toApplyDate)
  }

  return date
}
const range = require('sequelize/lib/dialects/postgres/range')

// console.log(Sequelize)
Sequelize.DataTypes.postgres.RANGE.prototype._stringify = function _stringify (values, options) {
  if (!Array.isArray(values)) {
    return "'" + this.options.subtype.stringify(values, options) + "'::" +
      this.toCastType()
  }
  const valuesStringified = values.map(value => {
    if (includes([null, -Infinity, Infinity], value)) {
      // Pass through "unbounded" bounds unchanged
      return value
    } else if (this.options.subtype.stringify) {
      // console.log(value)
      return this.options.subtype.stringify(value, options)
    } else {
      return options.escape(value)
    }
  })

  // Array.map does not preserve extra array properties
  valuesStringified.inclusive = defaults(get(values, 'inclusive', map(values, 'inclusive')), [true, false]) // values.inclusive

  return '\'' + range.stringify(valuesStringified) + '\''
}

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Sep 14, 2018

But it use non standard array properties and likely will not pass our full test suite, I advice you to use proper fix from v5-beta once you get chance. Otherwise you may keep using your fix for your projects

@theRichu
Copy link

@theRichu theRichu commented Sep 14, 2018

Ok, i'll check and test with v5-beta. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants