feat: time with timezone

[Sasha-hk]

Pull Request Checklist

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

• Have you added new tests to prevent regressions?
• Does yarn test or yarn 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

Closes #14211

Create data type to store only time with timezone

[Sasha-hk]

I did have similar error when test the code locally. But it was with postgres dialect.

Can I fix this or can I just wait?

[WikiRik]

Don't worry about the failing DB2 tests, they are not related to your PR

[ephys]

Thank you for the PR :)
A few copy-paste errors, and one potentially failing scenario, but overall looking good

Could you also add DataTypes.TIME.ZONED in the documentation? https://github.com/sequelize/sequelize/blob/main/docs/manual/other-topics/other-data-types.md

[Sasha-hk]

Okay I add documentation

[ephys]

I forgot one last test: the integration one

It tests that the value you put in is properly serialized/deserialized when sent/retrieved from the database.

It goes in test/integration/data-types.test.js
You can take inspiration from the TIME one (no need to use moment):

sequelize/test/integration/data-types.test.js

Line 162 in 85b0984

it('calls parse and stringify for TIME', async () => {

The test must only be enabled for dialects that support TIME WITH TIMEZONE, we typically do that by adding a flag on the dialect:

• example of a test enabled for dialects that support a specific feature:

  sequelize/test/integration/data-types.test.js

  Line 369 in 85b0984

  if (current.dialect.supports.TSVECTOR) {

• where the default value for that example flag is set: https://github.com/sequelize/sequelize/blob/main/src/dialects/abstract/index.ts#L172
• its typings: https://github.com/sequelize/sequelize/blob/main/src/dialects/abstract/index.ts#L87
• and how postgres turns on that flag: https://github.com/sequelize/sequelize/blob/main/src/dialects/postgres/index.js#L49

I'd recommend naming the flag TIME_WITH_TIME_ZONE

This may be a lot in one go but I'm pretty sure it's the last thing :)

[Sasha-hk]

If I need to update something, I do it. Thank you for patience :)

[ephys]

I'm not sure why test integration test is failing (to be honest it's the first time I look into that test suite). I'll assign this to myself to remind myself to take a look into it when I'm available :)

[Sasha-hk]

About when will the new version of Sequelize be released?

[ephys]

The new v6 release that includes this? Not sure, I need to find the time to figure out how to write this test

[Sasha-hk]

I'm just wondering when I will be able to use the data type. I created the data type locally so as not to stop the development of my project. So take your time :)

[WikiRik]

I'm not sure why test integration test is failing (to be honest it's the first time I look into that test suite). I'll assign this to myself to remind myself to take a look into it when I'm available :)

I think we might need to rewrite the testSuccess function there. I don't think it can deal with the { zoned: true }. Might be best for now to just rewrite the test so it does more in the test itself and it doesn't use the testSuccess function.

[Sasha-hk]

I tried to write the test, but I don't know how else to implement it.
The parse method is not called

Oh, I got it, I'll try again

[Sasha-hk]

I don't know if it's another error or if it caused an error before

Error:

1) [POSTGRES] DataTypes
       calls parse and stringify for TIME WITH TIMEZONE:
     invalid input syntax for type time with time zone: "{}"
  Error
      at Query.run (lib/dialects/postgres/query.js:55:25)
      at /home/petryk/dev/sequelize/lib/sequelize.js:321:28
      at processTicksAndRejections (node:internal/process/task_queues:96:5)
      at async PostgresQueryInterface.insert (lib/dialects/abstract/query-interface.js:297:21)
      at async model.save (lib/model.js:2442:35)
      at async Function.create (lib/model.js:1348:12)
      at async Context.<anonymous> (test/integration/data-types.test.js:199:7)

Test:

  if (current.dialect.supports.TIME_WITH_TIME_ZONE) {
    it('calls parse and stringify for TIME WITH TIMEZONE', async () => {
      const Type = new Sequelize.TIME({ zoned: true });
      const value = '12:00:00 +02:00';
      const options = {};
      let parseHaveBeenCalled = false;
      let stringifyHaveBeenCalled = false;

      // await testSuccess(Type, moment(new Date()).format('hh:mm:ss ZZ'));
      Type.constructor.parse = value => {
        parseHaveBeenCalled = true;

        return value;
      };

      Type.constructor.prototype.stringify = () => {
        stringifyHaveBeenCalled = true;

        return Reflect.apply(Sequelize.ABSTRACT.prototype.stringify, this, arguments);
      };

      const User = current.define('user', {
        field: Type,
      }, {
        timestamps: false,
      });

      await current.sync({ force: true });

      current.refreshTypes();

      await User.create({ // <<< error here
        field: value,
      });

      await User.findAll();
      expect(parseHaveBeenCalled).to.be.equal(true);
      expect(stringifyHaveBeenCalled).to.be.equal(true);

      delete Type.constructor.parse;
      delete Type.constructor.prototype.stringify;
    });

[ephys]

I just resolved the conflicts. If it's ok with @Sasha-hk I'll take over the branch to look into the test issue

[Sasha-hk]

Okay

[ephys]

Ok so the issue is that data-types.test is only there to check that a parse method is called on the DataType.

It's nowhere nearly as useful as testing that the value is correctly serialized/deserialized when interacting with the database.

I'll rewrite the test suite in a follow-up PR, after #14373 has been merged.

In the meantime, this PR doesn't do anything weird, so it has my approval

[WikiRik]

@ephys do you want to leave this open until you start on rewriting the test suite? (after #13799 has been merged probably) or can we merge this soon?

[WikiRik]

@ephys this one is blocked by #14505 I assume?

[ephys]

Yep! That branch revamps the test suite completely

[ephys]

I've done a lot more data type research in the past months with the data type rewrite and I think we should not include support for TIMETZ. From my research, it is a useless type that can only bring confusion. The postgres wiki is very clear about the fact that it should never be used.

I'm sorry it comes after all this effort, but I propose to abandon this feature

[Sasha-hk]

I've done a lot more data type research in the past months with the data type rewrite and I think we should not include support for TIMETZ. From my research, it is a useless type that can only bring confusion. The postgres wiki is very clear about the fact that it should never be used.

I'm sorry it comes after all this effort, but I propose to abandon this feature

I agree with you, we can close this PR