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

fix mssql and r2dbc integration #15264

Merged
merged 5 commits into from
Jun 9, 2021

Conversation

atomfrede
Copy link
Member

This PR fixes all issues mentioned in #15032

  • using asEntity('User') everywhere
  • correcting the mssql r2dbc url
  • adding the necessary setting to liquibase migration

closes #15032


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

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

@atomfrede atomfrede force-pushed the 15032-mssql-user-entity-suffix branch from 29f1b4a to e54bfbe Compare June 8, 2021 19:29
…e/changelog/initial_schema.xml.ejs

Co-authored-by: Daniel Franco <dandrfranco@gmail.com>
@atomfrede atomfrede requested a review from DanielFran June 9, 2021 09:24
@DanielFran
Copy link
Member

@atomfrede @jhipster/developers We are using an explicit liquibase extension sabomichal/liquibase-mssql that allow defining the property identityInsertEnabled="true" to allows explicit values to be inserted into the identity column of a table, so the same as defining "SET IDENTITY_INSERT jhi_user ON/OFF".
The use of this specific extension was due to no support for this MSSQL property in 2009: https://liquibase.jira.com/wiki/spaces/CONTRIB/pages/1998867/MS+SqlServer+Extensions

In the meantime, the support of official MSSQL is up-to-date, so we probably should migrate back to the official liquibase extension.

So there are 2 possibilities:
1 - exclude the use of the property "identityInsertEnabled="true"" explicitly for reactive and define "SET IDENTITY_INSERT jhi_user ON/OFF" only for reactive
2 - Drop use of sabomichal/liquibase-mssql in favour of the liquibase/liquibase-mssql and define "SET IDENTITY_INSERT jhi_user ON/OFF" like in the official extension for both reactive and non-reactive database

@atomfrede
Copy link
Member Author

I would say let's use the official extension.

@atomfrede
Copy link
Member Author

But very strange it doesn't work with identityInsertEnabled property.

@DanielFran
Copy link
Member

DanielFran commented Jun 9, 2021

Liquibase version we are using is version 4 and actually the extension was compatible with version 3, not sure there is any issue with the version 4.

Anyway Liquibase should handle now reactive database also, so I agree the better is to switch with liquibase official extension.

@atomfrede
Copy link
Member Author

@DanielFran Done. Works fine (tested with reactive only though, but don't see why it should not work with classical jdbc).

@DanielFran
Copy link
Member

@atomfrede Can you please review tests?

@atomfrede
Copy link
Member Author

@atomfrede Can you please review tests?

On it. Seems easy to fix.

…owmapper/UserRowMapper.java.ejs

Co-authored-by: Marcelo Shima <marceloshima@gmail.com>
@DanielFran
Copy link
Member

DanielFran commented Jun 9, 2021

@atomfrede LGTM

Maybe we need to delete https://www.jhipster.tech/tips/004_tip_using_ms_sql_server.html

I will take care of the bom

@DanielFran DanielFran merged commit a44630f into jhipster:main Jun 9, 2021
DanielFran added a commit to jhipster/jhipster-bom that referenced this pull request Jun 9, 2021
@atomfrede
Copy link
Member Author

Bounty claimed https://opencollective.com/generator-jhipster/expenses/42719.

Regarding the tip, there is already a disclaimer that the tip is not required anymore. But maybe it is time to delete it now.

@pascalgrimaud
Copy link
Member

@atomfrede : approved

@pascalgrimaud pascalgrimaud added this to the 7.1.0 milestone Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants