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

Support all hibernate-envers configuration options #21872

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

Naros
Copy link
Contributor

@Naros Naros commented Dec 2, 2021

Hi @Sanne and @gsmet, this PR enables all hibernate-envers configuration options for use in Quarkus.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I spotted something that needs an addition.

@Naros Naros requested a review from gsmet December 2, 2021 13:43
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Better, thanks!

@gsmet
Copy link
Member

gsmet commented Dec 2, 2021

@Sanne I don't know if you want to have a look at this one?

@Sanne
Copy link
Member

Sanne commented Dec 2, 2021

@Sanne I don't know if you want to have a look at this one?

If you looked I know it's good :)

Just the obligatory question for @Naros .. are you sure all of these need to be exposed? Any default that should be different than upstream Envers?

Remember that Quarkus intentionally targets new applications; we favour having the best defaults over backwards compatibility with legacy platforms.
It's a good chance to revisit the properties before you expose them, to think if some default should have been different, and perhaps not exposed at all.

(In Hibernate we tend to add new flags to opt-in into new behaviours when they are not backwards compatible - if you know of any such case with Envers it might be preferrable to opt-in by default and not even expose the knob to revert to the legacy behaviour; this is also useful to enable us to eventually remove the legacy stuff upstream)

@Naros
Copy link
Contributor Author

Naros commented Dec 3, 2021

Hi @Sanne

are you sure all of these need to be exposed?

I honestly think so.

Any default that should be different than upstream Envers?

The only one that might come to mind in terms of differing defaults would be modifiedColumnNamingStrategy. If the goal is to target new applications then it might be worthwhile to default to the improved strategy; still allowing it to be exposed though and users could toggle the legacy behavior through that setting if they want it.

Otherwise, I don't think there's any harm in exposing what is here. There are some new ones coming in Envers 6.0 that we'll need to consider adding here but I think I'd prefer those to bake upstream first before pulling them in here.

@Sanne
Copy link
Member

Sanne commented Dec 3, 2021

@Naros all good then 💯

Want me to merge this as-is or would you prefer to change the default for modifiedColumnNamingStrategy first ?

@gsmet
Copy link
Member

gsmet commented Dec 6, 2021

Let's merge. The Envers extension has already been there for a while so changing the default is an orthogonal discussion that might break existing applications. @Naros I'll let you decide what you want to do here. The good thing is that this patch allows users to override the default if you change it.

@gsmet gsmet merged commit aa53d46 into quarkusio:main Dec 6, 2021
@quarkus-bot quarkus-bot bot added this to the 2.6 - main milestone Dec 6, 2021
@Naros
Copy link
Contributor Author

Naros commented Dec 6, 2021

Hi guys! @gsmet brings up a great point that having all configs exposed does also freely allows us to change Envers defaults if the need arises and allows Quarkus users to adjust configs as necessary. We just have to be diligent about making sure that if this situation occurs that we document it upstream and make sure to make the adjustments in the Quarkus migration guide accordingly.

@Sanne
Copy link
Member

Sanne commented Dec 6, 2021

Sure, we can update defaults later but if there's anything you'd want to change now that would be even better :)

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

Successfully merging this pull request may close these issues.

3 participants