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

Postgres range inclusive property lost in JSON format #8471

Closed
kingjerod opened this issue Oct 13, 2017 · 2 comments
Closed

Postgres range inclusive property lost in JSON format #8471

kingjerod opened this issue Oct 13, 2017 · 2 comments
Milestone

Comments

@kingjerod
Copy link

@kingjerod kingjerod commented Oct 13, 2017

What are you doing?

Querying a Postgres table with a daterange column and returning it as JSON.

const Timeline = Sequelize.define({
   id: {type: Sequelize.INTEGER, primaryKey: true},
   date_range: {type: Sequelize.RANGE(Sequelize.DATEONLY)}
});
const range = [
   {value: new Date(Date.UTC(2017, 0, 1)), inclusive: true},
   {value: new Date(Date.UTC(2017, 6, 1)), inclusive: false},
];
await Timeline.create({id: 1, date_range: range});
const row = await Timeline.findById(1);
console.log(JSON.stringify(row)); // No inclusive property

What do you expect to happen?

When returning the data as JSON, the inclusive data should not be lost.

In the docs, there is this optional format for updating, this would be better because it can be turned into JSON without losing any information:

const range = [
  { value: '2017-01-01', inclusive: false },
  { value: '2017-07-01', inclusive: true },
];

What is actually happening?

The inclusive property is lost. This is because the inclusive property is set on an array, and array properties are not converted into JSON (like Array.length).

{id:1, date_range: ['2017-01-01', '2017-07-01']}

Dialect: postgres
Dialect version: 9.6
Database version: 9.6
Sequelize version: 4.4.3
Tested with master branch: Yes

@stale
Copy link

@stale stale bot commented Jan 11, 2018

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 Jan 11, 2018
@stale stale bot closed this Jan 18, 2018
@sushantdhiman sushantdhiman reopened this Mar 24, 2018
@sushantdhiman sushantdhiman added this to the v5 milestone Mar 24, 2018
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
@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented May 7, 2018

Fixed by #9364

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.

2 participants