-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix #1080: Protection against duplicate entries in the pa_operation_template table #1091
Fix #1080: Protection against duplicate entries in the pa_operation_template table #1091
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move the liquibase to the next version, please. Otherwise good job.
...g/changesets/powerauth-java-server/1.5.x/20231018-add-constraint-operation-template-name.xml
Outdated
Show resolved
Hide resolved
...g/changesets/powerauth-java-server/1.5.x/20231018-add-constraint-operation-template-name.xml
Outdated
Show resolved
Hide resolved
...g/changesets/powerauth-java-server/1.5.x/20231018-add-constraint-operation-template-name.xml
Outdated
Show resolved
Hide resolved
...tlime/security/powerauth/app/server/database/repository/OperationTemplateRepositoryTest.java
Show resolved
Hide resolved
...curity/powerauth/app/server/service/behavior/tasks/OperationTemplateServiceBehaviorTest.java
Outdated
Show resolved
Hide resolved
</not> | ||
</preConditions> | ||
<comment>Add unique constraint to pa_operation_template.template_name</comment> | ||
<addUniqueConstraint tableName="pa_operation_template" columnNames="template_name" constraintName="pa_operation_template_template_name_uk" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zcgandcomp Should we keep recording the db changes also in the migration guides or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add information into the migration guide, please add this information in the usual way as seen in previous migration guides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be concrete about the changes or just mention that some changes are needed and link a script, which will be generated during the release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/wultra/development-internal/wiki/Using-Liquibase#13-migration-between-versions. Yes please add a migration guide and mention the change. In theory it is possible to have (invalid) duplicities in the DB. The migration script in SQL will be generated during release procedure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIce improvement, however we should also add step to the migration guide. We should also document how to proceed in case addition of the unique constraint fails.
</not> | ||
</preConditions> | ||
<comment>Add unique constraint to pa_operation_template.template_name</comment> | ||
<addUniqueConstraint tableName="pa_operation_template" columnNames="template_name" constraintName="pa_operation_template_template_name_uk" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/wultra/development-internal/wiki/Using-Liquibase#13-migration-between-versions. Yes please add a migration guide and mention the change. In theory it is possible to have (invalid) duplicities in the DB. The migration script in SQL will be generated during release procedure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The migration guide is good for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK.
Fix #1080