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

Add ability to add additional hibernate dialect for third party databases. #25792

Merged
merged 6 commits into from May 30, 2022

Conversation

alexeysharandin
Copy link
Contributor

At this moment all Hibernates dialects hardcoded in io.quarkus.hibernate.orm.deployment.Dialects

This PR first step of resolving comment in this class:
// For now select the latest dialect from the driver
// later, we can keep doing that but also avoid DCE
// of all the dialects we want in so that people can override them

@quarkus-bot
Copy link

quarkus-bot bot commented May 25, 2022

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@alexeysharandin alexeysharandin changed the title add ability to add additional hibernate dialect for third party databases. Add ability to add additional hibernate dialect for third party databases. May 25, 2022
@Sanne Sanne requested a review from yrodiere May 25, 2022 15:15
@quarkus-bot quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label May 25, 2022
Copy link
Member

@yrodiere yrodiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I added a few commits, mainly to:

  1. Move the newly introduced build item to a dedicated module for SPIs of the Hibernate ORM extension, because this really is about Hibernate ORM and not datasources/jdbc.
  2. Use the newly introduced build item for all core DB kinds. The main advantage is that we get testing of that build item for free, thanks to existing integration tests.

Unless @Sanne or @gsmet objects to the creation of a new module, let's merge this once CI agrees.

@Sanne
Copy link
Member

Sanne commented May 25, 2022

sounds good - no objections! Thanks :)

@yrodiere yrodiere added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 25, 2022
@quarkus-bot

This comment has been minimized.

alexeysharandin and others added 6 commits May 30, 2022 08:50
…e this - all dialects was hardcoded in Dialects.java
Because dialects are metadata specific to Hibernate ORM.
These tests are not absolutely necessary since we have integration
tests for each database kind, which would fail if we targeted a
non-existing dialect.

And we won't be able to (easily) execute those tests after the next
commit anyway.
@yrodiere yrodiere merged commit b640732 into quarkusio:main May 30, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 30, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 30, 2022
@yrodiere
Copy link
Member

Merged! Thanks again @alexeysharandin , and good luck with your extension :)

@gastaldi
Copy link
Contributor

gastaldi commented Jun 2, 2022

That is really cool! But the build item name isn't too friendly (DatasourceDbKindHibernateOrmMetadataBuildItem 😅 ) for two reasons:

  • The class is already in a package with Hibernate in its name, I don't see why adding Hibernate in the name would help;
  • This is just a DbKind/Dialect tuple. If another Hibernate ORM metadata specific attribute is necessary, the constructor may need to be changed or a builder would need to be introduced.

How about naming it simply io.quarkus.hibernate.orm.deployment.spi.DatabaseKindDialectBuildItem?

UPDATE: Just saw that the original class was io.quarkus.datasource.deployment.spi.DatabaseKindBuildItem. Better add an entry in the migration guide about that.

UPDATE 2: I created #25918, feel free to reject/close the PR if that doesn't make sense 😉

gastaldi added a commit to gastaldi/quarkus that referenced this pull request Jun 2, 2022
gastaldi added a commit to gastaldi/quarkus that referenced this pull request Jun 2, 2022
gastaldi added a commit to gastaldi/quarkus that referenced this pull request Jun 2, 2022
@yrodiere
Copy link
Member

yrodiere commented Jun 6, 2022

UPDATE: Just saw that the original class was io.quarkus.datasource.deployment.spi.DatabaseKindBuildItem. Better add an entry in the migration guide about that.

That is a different build item. We didn't rename an existing build item, we added another one specifically for Hibernate ORM metadata.

This is just a DbKind/Dialect tuple. If another Hibernate ORM metadata specific attribute is necessary, the constructor may need to be changed or a builder would need to be introduced.

Yes we would need another constructor. Which is much easier to do in a backwards compatible way than renaming the class. And that's something we've done multiple times in the Hibernate ORM extension.

The class is already in a package with Hibernate in its name, I don't see why adding Hibernate in the name would help;

That I can agree with : the name is very long 😀 Though I'm not sure the renaming is worth the trouble now that it's been merged and @alexeysharandin is probably already trying to use it.

@alexeysharandin
Copy link
Contributor Author

@yrodiere don't worry about me and naming. I'm waiting when this PR will be included to Quarkus 2.x and only after this can use this BuildItem. In this case - naming up to you :)

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.

None yet

4 participants