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

Liquibase foreign key relationship column names #27608

Merged

Conversation

OmarHawk
Copy link
Contributor

@OmarHawk OmarHawk commented Oct 17, 2024

Fixes #27598 and adds additional conditions to the existing tests (which were missing)


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@mraible
Copy link
Contributor

mraible commented Oct 17, 2024

Can you please run npm run update-snapshots and commit/push the changes?

@OmarHawk
Copy link
Contributor Author

at the moment, there is no change in the snapshots as I haven't changed anything in the test case (jdl definition) itself, I just added additional conditions into the tests to verify the output of the current tests.

But I can adjust the template / generation logic now, which would then in turn actually change the snapshots

@mraible
Copy link
Contributor

mraible commented Oct 17, 2024

@OmarHawk If you run npm test, you should see the errors. Updating the snapshots might fix them, that's why I suggested it.

@OmarHawk
Copy link
Contributor Author

ok, understood, but I considered that already. :-)

@OmarHawk OmarHawk force-pushed the bugfix/liquibase-foreign-key-recreate branch from fde1fd7 to 466e789 Compare October 18, 2024 13:26
@OmarHawk
Copy link
Contributor Author

I have moved the working logic from the add constraints into an own partial template, and use that to be included in both the add and update contraints

@OmarHawk OmarHawk marked this pull request as ready for review October 18, 2024 14:36
@mshima
Copy link
Member

mshima commented Oct 18, 2024

Liquibase changelog should be polished, you can see in ng-default sample compare:

a/src/main/resources/config/liquibase/changelog/20160208210109_added_entity_constraints_TestCustomTableName.xml
  @@ -10,6 +10,9 @@
       -->
       <changeSet id="20160208210109-2" author="jhipster">
   
  +
  +
  +
           <addForeignKeyConstraint baseColumnNames="test_entity_id"
                                    baseTableName="test_custom_table_name_entity"
                                    constraintName="fk_test_custom_table_name_entity__test_entity_id"
  diff --git b/src/main/resources/config/liquibase/changelog/20160208210110_added_entity_constraints_TestEntity.xml a/src/main/resources/config/liquibase/changelog/20160208210110_added_entity_constraints_TestEntity.xml
  index e4385fd..dcb84bf 100644
  --- b/src/main/resources/config/liquibase/changelog/20160208210110_added_entity_constraints_TestEntity.xml
  +++ a/src/main/resources/config/liquibase/changelog/20160208210110_added_entity_constraints_TestEntity.xml
  @@ -10,6 +10,9 @@
       -->
       <changeSet id="20160208210110-2" author="jhipster">
   
  +
  +
  +
           <addForeignKeyConstraint baseColumnNames="user_one_to_many_id"
                                    baseTableName="test_entity"
                                    constraintName="fk_test_entity__user_one_to_many_id"
  @@ -37,5 +40,8 @@

@OmarHawk OmarHawk force-pushed the bugfix/liquibase-foreign-key-recreate branch from 466e789 to 73bef2b Compare October 18, 2024 15:57
@OmarHawk
Copy link
Contributor Author

Liquibase changelog should be polished, you can see in ng-default sample compare:

I see, will fix it :-)

@OmarHawk
Copy link
Contributor Author

I do have now less empty lines, do you think, it is alright @mshima? Or does it have to be the exact same way as before?

DanielFran
DanielFran previously approved these changes Oct 18, 2024
@mshima
Copy link
Member

mshima commented Oct 18, 2024

I think we should keep the same blank lines to avoid unnecessary diff when upgrading applications, specially changelogs.

@OmarHawk OmarHawk force-pushed the bugfix/liquibase-foreign-key-recreate branch 2 times, most recently from 1287530 to b06f220 Compare October 21, 2024 06:54
@OmarHawk
Copy link
Contributor Author

I think we should keep the same blank lines to avoid unnecessary diff when upgrading applications, specially changelogs.

Added the empty line again in the add constraints changelog file. Hope it looks alright now :-)

@OmarHawk
Copy link
Contributor Author

OmarHawk commented Oct 21, 2024

ah, damn, as soon, as I add the empty line into the for loop, it creates some additonal lines for unhandled relationships. So I would need to pass a paramater to add additional empty lines into the new template just for the add case. I don't think, that is a good idea. I'd rather keep it now clean and without these empty lines at all.

@OmarHawk OmarHawk force-pushed the bugfix/liquibase-foreign-key-recreate branch from b06f220 to a33b5b2 Compare October 21, 2024 08:31
@OmarHawk
Copy link
Contributor Author

Reverted the last change regarding the empty line in the add template - please check the Merge Request again @DanielFran, @mshima , whether you are OK with it: I really don't think it makes sense to have a parameter named createEmptyLines: true as part of a template in order to have empty lines in add case, but not in the update case.
The benefit of having it combined into one template is larger than having some (even less!) useless empty lines.

@mshima
Copy link
Member

mshima commented Oct 21, 2024

Updated files are not overridden.
There is no problem on changing format.

@OmarHawk
Copy link
Contributor Author

ok, interesting that after the change it shows no changes anymore now. I'd have assumed that with adding the empty lines within the template, it would now produce empty lines in the update constraint files...

@OmarHawk
Copy link
Contributor Author

OmarHawk commented Oct 21, 2024

I'll update the snapshots once more, if you are OK with it - then the MR should become green :-)

@mshima
Copy link
Member

mshima commented Oct 21, 2024

@OmarHawk projects are identical now, should be ok to merge.
Please update snapshots.

@OmarHawk OmarHawk force-pushed the bugfix/liquibase-foreign-key-recreate branch from 736780d to 88e1dac Compare October 21, 2024 11:48
@OmarHawk
Copy link
Contributor Author

Done and squashed the excess commits so that the merge contains only a single commit and looks cleaner.

@mshima mshima merged commit 2081ef1 into jhipster:main Oct 21, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Liquibase: Incorrect creation of new foreign key relationships during schema updates
4 participants