HHH-20641 @AuditOverride / @AuditOverrides - #13086
Conversation
|
Thanks for your pull request! This pull request does not follow the contribution rules. Could you have a look? ❌ All commit messages should start with a JIRA issue key matching pattern › This message was automatically generated. |
cf3c0e4 to
6b35a3b
Compare
bf64069 to
1c8cbf9
Compare
1c8cbf9 to
6ac6d23
Compare
mbellade
left a comment
mbellade
left a comment
There was a problem hiding this comment.
Thanks @niklas-enns, left a first round of review comments
| * The {@code AuditingOverride} annotation is used to override the auditing | ||
| * behavior of a superclass or single property inherited from {@link jakarta.persistence.MappedSuperclass} | ||
| * type, or attribute inside an embedded component. | ||
| * | ||
| * @author Erik-Berndt Scheper | ||
| * @author Lukasz Antoniak (lukasz dot antoniak at gmail dot com) | ||
| * @author Niklas Enns |
There was a problem hiding this comment.
Perhaps let's expand the javadoc of this annotation a bit, to specify when its useful (overriding audit-specific behavior in inheritance hierarchy), and what can be done with it.
Also, please drop @author tags, we don't use them anymore.
There was a problem hiding this comment.
I added two example sentences. Should I add example code snippes aswell?
| * @author Niklas Enns | ||
| */ | ||
|
|
||
| public record AuditTableConfig(String name, String schema, String catalog, String changesetIdColumn, |
There was a problem hiding this comment.
I fail to see why this new record type is useful, both @Audited.Table and the AuditOverride annotation usage instances already contain all values within them, instantiating a new wrapper around them seems wasteful.
There was a problem hiding this comment.
Both methods AuditHelper.bindAuditTable and AuditHelper.bindSubclassAuditTables have to set the final config for the audit table (name, schema, catalog etc...) which can come from an Audited.Table annotation or the new Audited.Override annotation. Without the introduced AuditTableConfig, we would have to pass all the ingredients to both methods and then make the decision there.
With AuditTableConfig, the decision of which annotation values to finally take is moved to an earlier stage and location (its now directly within the static factory methods of the AuditTableConfig class). The consumer methods AuditHelper.bindAuditTable and AuditHelper.bindSubclassAuditTables can now just take the value without caring about an override at all.
Maybe this abstract description is hard to imagine or not convincing you. Should I provide an implementation proposal without the new record type? Maybe it gets clearer when we can compare both approaches side by side.
There was a problem hiding this comment.
It just seems to me that the only thing we can override is the property-level CollectionTable of a *-to-many association, but you're trying to wrap all audited table binding with this new record, which to me is confusing at best.
I would prefer a single new parameter in the collection binding path only, e.g.:
static void bindOneToManyAuditTable(
@Nullable Audited.Table auditTable,
Collection collection,
String referencedEntityName,
@Nullable Audited.CollectionTable collectionAuditTable,
@Nullable Audited.CollectionTable collectionTableOverride,
MetadataBuildingContext context) { But I understand there might be some confusion: at this time, CollectionTable is only supported on @OneToManys, and @ManyToMany/@ElementCollections don't work properly; I believe the original intention was to make @Audited.Table work at a property-level too, but at this time that's not possible because of its @Target missing FIELD/METHOD.
I would say that is an existing limitation, which might deserve a follow-up PR, but we can leave it be at this time for the overrides only.
mbellade
left a comment
mbellade
left a comment
There was a problem hiding this comment.
Thanks again @niklas-enns, another round of comments.
| .filter( Audited.Override::isAudited ).collect( Collectors.toSet() ); | ||
| } | ||
|
|
||
| static Audited.Override findFirstAuditOverrideForProperty(PersistentClass rootClass, String name, MetadataBuildingContext context) { |
There was a problem hiding this comment.
This is still being called once for every property, scanning the entire hierarchy multiple times unnecessary. I would instead compute a Map<String, Audit.Override> per-class, only once, before binding all properties - consulting that when needed here, instead of going through through annotation scanning every time. Something like:
private static Map<String, Audited.Override> collectAuditOverrides(
RootClass rootClass,
MetadataBuildingContext context) {
var registry = context.getBootstrapContext().getModelsContext().getClassDetailsRegistry();
var overrides = new HashMap<String, Audited.Override>();
registry.getClassDetails( rootClass.getClassName() ).forEachAnnotationUsage( Audited.Override.class, modelsContext,
override -> overrides.putIfAbsent( override.name(), override )
);
// probably need something similar for any `MappedSuperclass` here
for ( var subclass : rootClass.getSubclasses() ) {
registry.getClassDetails( subclass.getClassName() ).forEachAnnotationUsage( Audited.Override.class, modelsContext,
override -> overrides.putIfAbsent( override.name(), override )
);
}
return overrides;
}| * @author Niklas Enns | ||
| */ | ||
|
|
||
| public record AuditTableConfig(String name, String schema, String catalog, String changesetIdColumn, |
There was a problem hiding this comment.
It just seems to me that the only thing we can override is the property-level CollectionTable of a *-to-many association, but you're trying to wrap all audited table binding with this new record, which to me is confusing at best.
I would prefer a single new parameter in the collection binding path only, e.g.:
static void bindOneToManyAuditTable(
@Nullable Audited.Table auditTable,
Collection collection,
String referencedEntityName,
@Nullable Audited.CollectionTable collectionAuditTable,
@Nullable Audited.CollectionTable collectionTableOverride,
MetadataBuildingContext context) { But I understand there might be some confusion: at this time, CollectionTable is only supported on @OneToManys, and @ManyToMany/@ElementCollections don't work properly; I believe the original intention was to make @Audited.Table work at a property-level too, but at this time that's not possible because of its @Target missing FIELD/METHOD.
I would say that is an existing limitation, which might deserve a follow-up PR, but we can leave it be at this time for the overrides only.
|
….java Co-authored-by: Marco Belladelli <marcobladel@gmail.com>
2 participants
Implemented @Auditoverrides and @AuditOverride
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.
Please make sure that the following tasks are completed:
Tasks specific to HHH-20641 (Sub-task):
documentation/src/main/asciidoc/userguidefor all features,documentation/src/main/asciidoc/introductionfor main features, links from existing documentationmigration-guide.adoc(breaking changes) andwhats-new.adoc(new features/improvements)https://hibernate.atlassian.net/browse/HHH-20641