fix(data-types): moment object throwing error #13818

fzn0x · 2021-12-23T14:25:08Z

Pull Request Checklist

Please make sure to review and check all of these items:

Have you added new tests to prevent regressions?
Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you update the typescript typings accordingly (if applicable)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

Fix #13816 , wrong if else and replace date checking with isMoment.

Todos

fix(data-types): moment object throwing error

WikiRik · 2021-12-23T14:41:16Z

Can we add regression tests for this? So one inputting a JS Date and one with a Moment object to check that this works without errors?

sdepold · 2021-12-23T15:58:55Z

I think we should have a test that verifies that you can pass a moment instance.

edit: ... what Rik said :D

fzn0x · 2021-12-23T17:04:41Z

Hello guys already pinged you about the regressed tests in slack, kindly checked it ;)

cc @sdepold @WikiRik

sdepold · 2021-12-26T19:30:48Z

I have added a test case but it is blowing up since the code change does not seem to fix the problem

WikiRik · 2021-12-26T21:26:27Z

@sdepold might be good to revert #13712 in the v6 branch for now since it was just a minor change to get rid of a deprecation warning (and with moment being deprecated itself not to worry that it will be removed). This second change looks too big to me and with timezone things you always need to be careful

And then later we can see if @fncolon can find a better fix that works with different types of Date-related objects

sdepold · 2021-12-27T07:22:52Z

I agree

fzn0x · 2021-12-27T08:01:57Z

I have added a test case but it is blowing up since the code change does not seem to fix the problem

I tried to find the blowing up parts, can you give me the specific result?

fzn0x · 2021-12-27T08:02:09Z

@sdepold might be good to revert #13712 in the v6 branch for now since it was just a minor change to get rid of a deprecation warning (and with moment being deprecated itself not to worry that it will be removed). This second change looks too big to me and with timezone things you always need to be careful

And then later we can see if @fncolon can find a better fix that works with different types of Date-related objects

Agree! 😃

sdepold · 2021-12-27T11:43:43Z

Actually I'm confused now myself. Let me have a look.

sdepold · 2021-12-27T11:44:41Z

Local:

       findAll
         special where conditions/smartWhere object
           should be able to find a row using greater than or equal to logic with moment dates:
     Error: Invalid value Moment<2013-01-09T00:00:00+01:00>
      at Object.escape (lib/sql-string.js:65:11)
      at MySQLQueryGenerator.escape (lib/dialects/abstract/query-generator.js:1034:22)
      at MySQLQueryGenerator._whereParseSingleValueObject (lib/dialects/abstract/query-generator.js:2679:41)
      at MySQLQueryGenerator.whereItemQuery (lib/dialects/abstract/query-generator.js:2389:21)
      at /Users/sdepold/Projects/sequelize/lib/dialects/abstract/query-generator.js:2290:25
      at Array.forEach (<anonymous>)
      at MySQLQueryGenerator.whereItemsQuery (lib/dialects/abstract/query-generator.js:2288:35)
      at MySQLQueryGenerator.getWhereConditions (lib/dialects/abstract/query-generator.js:2706:19)
      at MySQLQueryGenerator.selectQuery (lib/dialects/abstract/query-generator.js:1365:28)
      at MySQLQueryInterface.select (lib/dialects/abstract/query-interface.js:979:27)
      at Function.findAll (lib/model.js:1764:47)
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at Context.<anonymous> (test/integration/model/findAll.test.js:391:23)```

sdepold · 2021-12-27T11:46:10Z

oh I forgot to build the project 👌

sdepold · 2021-12-27T11:49:29Z

Alright. So, it works just fine locally (as in the tests are green). Should we merge the change or rather revert to the old solution. I'm fine with both really. Since this might be one of the last versions of sequelize 6 it might be worth fixing the warning. However, if we feel it's too unsafe, reverting is also ok.

* fix(data-types): moment object throwing error * fix: typo * fix: revert with consistent variable naming * test(moment): add moment support test case Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> Co-authored-by: Sascha Depold <sascha@depold.com>

github-actions · 2021-12-27T19:41:06Z

🎉 This PR is included in version 7.0.0-alpha.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2022-01-04T10:14:07Z

🎉 This PR is included in version 7.0.0-alpha2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

ephys · 2022-04-18T09:35:01Z

So I noticed this change when reviewing #14400, and this PR + #13712 sound very strange to me. Why would we only apply the timezone option only to non-moment things?

Isn't that a breaking change? Before this change, the timezone option was applied to all dates indiscriminately of which library created the date.

With time zones it's very likely that this breaking change would have gone unnoticed.

fzn0x · 2022-04-18T13:07:55Z

So I noticed this change when reviewing #14400, and this PR + #13712 sound very strange to me. Why would we only apply the timezone option only to non-moment things?

Isn't that a breaking change? Before this change, the timezone option was applied to all dates indiscriminately of which library created the date.

With time zones it's very likely that this breaking change would have gone unnoticed.

It is actually throwing error before this changes for some cases

What I did was to follow the default of how we handle timezone and date and then I found out we can't applied to all dates if the filter was using momentjs, so that's why this error did happen. Before this change, the timezone option was applied to all dates indiscriminately of which library created the date was passing to momentjs function and returning some unexpected errors. (#13816).

fzn0x · 2022-04-18T13:15:04Z

But I agree with you, this should be breaking change also we didn't applied all dates indiscrimantely correctly before, so we need much further changes to fix this issue #13816 without breaking changes, or accept the breaking change (since no one complain about if there's any usage turns out to be breaking)

ephys · 2022-04-18T13:24:06Z

It is actually throwing error before this changes for some cases

Because of #13712, or before that PR too? Because that PR introduced the breaking change too.

since no one complain about if

It's only been a couple of months since this was published and a subtle change like this would not cause an error to be thrown. You'd have to look at the data to realize something changed.

Either way, I think both of these PRs should be reverted. If all they did was fixed the warning from moment, we should look into fixing the warning in a non-breaking way.

fzn0x · 2022-04-18T13:26:33Z

It is actually throwing error before this changes for some cases

Because of #13712, or before that PR too? Because that PR introduced the breaking change too.

since no one complain about if

It's only been a couple of months since this was published and a subtle change like this would not cause an error to be thrown. You'd have to look at the data to realize something changed.

Either way, I think both of these PRs should be reverted. If all they did was fixed the warning from moment, we should look into fixing the warning in a non-breaking way.

before that PR too

ephys · 2022-04-18T13:29:08Z

Do you have more info about the error that was being thrown prior to #13712?

fzn0x · 2022-04-18T13:40:50Z

Do you have more info about the error that was being thrown prior to #13712?

I don't have any but that's actually not correctly way of applyTimezone and one of the issue report is the one above

ephys · 2022-04-18T13:57:36Z

This one #13816?

But that issue was caused by #13712, so it's not prior to it.

but that's actually not correctly way of applyTimezone

Why?

fzn0x · 2022-04-18T14:00:57Z

This one #13816?

But that issue was caused by #13712, so it's not prior to it.

but that's actually not correctly way of applyTimezone

Why?

Not that one, I mean PR sorry, applyTimezone throws warning when the input is not supported by moment.

fzn0x · 2022-04-18T14:01:21Z

This one #13816?

But that issue was caused by #13712, so it's not prior to it.

but that's actually not correctly way of applyTimezone

Why?

The reason is we need to use proper timezone on applyTimezone

ephys · 2022-04-18T14:10:33Z

The reason is we need to use proper timezone on applyTimezone

I'm sorry I don't understand what you mean, what was incorrect with applyTimezone before?

applyTimezone throws warning when the input is not supported by moment.

A warning, not an error. So it must be resolved in a backward compatible way.

The current fix that skips applyTimezone if it's already a moment object is not backward compatible as we end up with different values being stored:

Do you remember in which scenarios moment logged that warning?

fzn0x · 2022-04-18T14:27:16Z

The reason is we need to use proper timezone on applyTimezone

I'm sorry I don't understand what you mean, what was incorrect with applyTimezone before?

applyTimezone throws warning when the input is not supported by moment.

A warning, not an error. So it must be resolved in a backward compatible way.

The current fix that skips applyTimezone if it's already a moment object is not backward compatible as we end up with different values being stored:

Do you remember in which scenarios moment logged that warning?

It's incorrect because we use applyTimezone without filtering if it was moment compatible date or not so it will throwing a warning that might we didn't want to see and don't know how to hide it, but unfortunately like you said it is introducing breaking change (but I didn't see anyone notice if it was a breaking change).

ephys · 2022-04-18T14:44:29Z

It's incorrect because we use applyTimezone without filtering if it was moment compatible date or not so it will throwing a warning that might we didn't want to see and don't know how to hide it

As I said, the warning should be fixed, but in a backward compatible way.
Which is why I asked if you remembered in which scenarios the warning was logged. I can't manage to replicate it by just passing a moment object to the old function.

I didn't see anyone notice if it was a breaking change

As I said earlier, it doesn't mean it did not introduce bugs that have gone unnoticed, which again is especially easy when it comes to time zones. Or that it won't introduce a bug for someone that has not updated to Sequelize > 6.12 (even if they already were on Sequelize 6), so this is really not a valid point.

fzn0x · 2022-04-18T15:01:44Z

Indeed :), I was not minding for you to revert it, I will search up how to come up with a backward compatibility solution soon.

fzn0x · 2022-04-18T15:08:40Z

I actually didn't input anything when the warning was thrown, it was thrown when I was getting data with the DATE type attribute.

fzn0x · 2022-04-18T15:12:29Z

This is the workaround

  const crowdFundings = await CrowdFunding.findAndCountAll({
    distinct: true, // don't count the include part https://github.com/sequelize/sequelize/pull/4016#issuecomment-116294996
    attributes: [
     'date_attributes' // <- will cause the warning from momentjs
    ],
    include: [{ all: true }],
    where: {
      ...data,
      [Op.or]: searchUtil.search(crowdFundingAttributes, query.q || ""),
    },
    ...pagination.getPaginationOptionsData(),
  });

model.js

     date_attributes: {
       type: DataTypes.DATE,
       allowNull: false,
     },

ephys · 2022-04-18T19:42:50Z

Thanks, I'll see if I can reproduce the error!

WikiRik · 2022-04-21T15:27:40Z

@ephys did you reproduce it? Do we need to revert it in v6 or shall we just keep it this way until we get complaints?

ephys · 2022-04-22T07:11:24Z

I haven't taken the time to reproduce the initial warning yet, but I did test the behavior before & after this change and it does produce different datetime strings.

It produces the same UTC time, but using different UTC offsets, e.g.
2022-04-18 23:05:05.617 +09:00 and 2022-04-18 16:05:05.617 +02:00

This may be an issue in itself for some applications. It requires the user to be using both moment and the timeZone option in the sequelize constructor, which is probably going to be a small percentage of users.

fzn0x · 2022-04-24T13:48:39Z

I haven't taken the time to reproduce the initial warning yet, but I did test the behavior before & after this change and it does produce different datetime strings.

It produces the same UTC time, but using different UTC offsets, e.g. 2022-04-18 23:05:05.617 +09:00 and 2022-04-18 16:05:05.617 +02:00

This may be an issue in itself for some applications. It requires the user to be using both moment and the timeZone option in the sequelize constructor, which is probably going to be a small percentage of users.

Thanks for the research

* fix(data-types): moment object throwing error * fix: typo * fix: revert with consistent variable naming * test(moment): add moment support test case Co-authored-by: Sascha Depold <sdepold@users.noreply.github.com> Co-authored-by: Sascha Depold <sascha@depold.com>

fix(data-types): moment object throwing error

c6f9add

fzn0x self-assigned this Dec 23, 2021

fzn0x requested review from WikiRik and sdepold December 23, 2021 14:25

This was referenced Dec 23, 2021

fix(data-types): unnecessary warning when getting data with DATE dataTypes #13712

Merged

Sequelize v6.12.0+ is throwing an Error when using a moment.js object in date fields #13816

Closed

fix: typo

bbc9448

fzn0x added 2 commits December 24, 2021 00:10

fix: revert with consistent variable naming

91eac37

Merge branch 'main' into fix/moment-object

8d36932

fzn0x added type: bug type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc. labels Dec 24, 2021

fzn0x marked this pull request as draft December 26, 2021 09:49

sdepold and others added 2 commits December 26, 2021 20:20

Merge branch 'main' into fix/moment-object

31ed2aa

test(moment): add moment support test case

10a70f3

Merge branch 'main' into fix/moment-object

34c48e9

sdepold marked this pull request as ready for review December 27, 2021 11:49

Merge branch 'main' into fix/moment-object

25f6c47

sdepold merged commit cdd1760 into main Dec 27, 2021

sdepold deleted the fix/moment-object branch December 27, 2021 18:54

github-actions bot added the released on @v7 label Dec 27, 2021

github-actions bot added the released on @alpha label Jan 4, 2022

ephys mentioned this pull request Apr 18, 2022

refactor: ♻️ migrate moment.js to dayjs #14400

Merged

6 tasks

fzn0x added the breaking change For issues and PRs. Changes that break compatibility and require a major version increment. label Apr 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(data-types): moment object throwing error #13818

fix(data-types): moment object throwing error #13818

fzn0x commented Dec 23, 2021

WikiRik commented Dec 23, 2021

sdepold commented Dec 23, 2021 •

edited

fzn0x commented Dec 23, 2021 •

edited

sdepold commented Dec 26, 2021 •

edited

WikiRik commented Dec 26, 2021

sdepold commented Dec 27, 2021

fzn0x commented Dec 27, 2021

fzn0x commented Dec 27, 2021

sdepold commented Dec 27, 2021

sdepold commented Dec 27, 2021

sdepold commented Dec 27, 2021

sdepold commented Dec 27, 2021

github-actions bot commented Dec 27, 2021

github-actions bot commented Jan 4, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022 •

edited

fzn0x commented Apr 18, 2022 •

edited by ephys

ephys commented Apr 18, 2022 •

edited

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022 •

edited

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022

fzn0x commented Apr 18, 2022

fzn0x commented Apr 18, 2022 •

edited

ephys commented Apr 18, 2022

WikiRik commented Apr 21, 2022

ephys commented Apr 22, 2022

fzn0x commented Apr 24, 2022

fix(data-types): moment object throwing error #13818

fix(data-types): moment object throwing error #13818

Conversation

fzn0x commented Dec 23, 2021

Pull Request Checklist

Description Of Change

Todos

WikiRik commented Dec 23, 2021

sdepold commented Dec 23, 2021 • edited

fzn0x commented Dec 23, 2021 • edited

sdepold commented Dec 26, 2021 • edited

WikiRik commented Dec 26, 2021

sdepold commented Dec 27, 2021

fzn0x commented Dec 27, 2021

fzn0x commented Dec 27, 2021

sdepold commented Dec 27, 2021

sdepold commented Dec 27, 2021

sdepold commented Dec 27, 2021

sdepold commented Dec 27, 2021

github-actions bot commented Dec 27, 2021

github-actions bot commented Jan 4, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022 • edited

fzn0x commented Apr 18, 2022 • edited by ephys

ephys commented Apr 18, 2022 • edited

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022 • edited

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022

ephys commented Apr 18, 2022

fzn0x commented Apr 18, 2022

fzn0x commented Apr 18, 2022

fzn0x commented Apr 18, 2022 • edited

ephys commented Apr 18, 2022

WikiRik commented Apr 21, 2022

ephys commented Apr 22, 2022

fzn0x commented Apr 24, 2022

sdepold commented Dec 23, 2021 •

edited

fzn0x commented Dec 23, 2021 •

edited

sdepold commented Dec 26, 2021 •

edited

fzn0x commented Apr 18, 2022 •

edited

fzn0x commented Apr 18, 2022 •

edited by ephys

ephys commented Apr 18, 2022 •

edited

fzn0x commented Apr 18, 2022 •

edited

fzn0x commented Apr 18, 2022 •

edited