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

BUG: migrate script sets the wrong APPLIED_AT value #202

Open
tohagan-tmr opened this issue Dec 1, 2021 · 3 comments
Open

BUG: migrate script sets the wrong APPLIED_AT value #202

tohagan-tmr opened this issue Dec 1, 2021 · 3 comments

Comments

@tohagan-tmr
Copy link
Contributor

The APPLIED_AT value is invalid since it should reflect the time the generated SQL script was executed rather than the time the migrate script code gen was run. It's important to have the correct value set for data management auditing.

If the APPLIED_AT field was a DATETIME value then it would be easier to fix this using generic SQL but since its VARCHAR we may need a property settings to specify the SQL expression used to convert NOW() to a VARCHAR value for APPLIED_AT as this can vary by SQL engine.

@harawata
Copy link
Member

harawata commented Dec 7, 2021

Hi @tohagan-tmr ,

It makes sense, but I cannot think of a solution that works with all (or most major) databases and does not break backward compatibility.

As a workaround, you may be able to use the 'script hook' to append UPDATE statement after each migration scripts.
The hook script would look something like this:

if (!hookContext.isUndo()) {
  print('UPDATE ' + changelog + " SET APPLIED_AT TO_CHAR(CURRENT_TIMESTAMP, 'YYYY-MM-DD...') WHERE ID = " + hookContext.getChange().getId());
  print('/');
}

Does this work for you?

FYI, you can test the 'script hook' using the latest 3.3.10 snapshots.
Please see the doc/tests attached to #206 for the usage.

@tohagan-tmr
Copy link
Contributor Author

Yeah that should be fine. Sad we can't fix it for the generic case.

Might be something to keep mind if you're ever contemplating a future major version. You could then convert APPLIED_AT to a DATETIME field and offer sample migration rules to convert the column for various database types.

Thanks very much for all your help this week. Greatly appreciated.

@harawata
Copy link
Member

harawata commented Dec 8, 2021

Might be something to keep mind if you're ever contemplating a future major version.

Will do!
Thank you fro the valuable feedback!

Note to self:
Changing the column data type may require extra initial configuration which is a huge minus.
The standard CURRENT_TIMESTAMP function can be used with a VARCHAR column, however, its output format varies between DB vendors and the current column size (=25) is too small.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants