HHH-20214 Full @Audited entities read/write support in core#12191
HHH-20214 Full @Audited entities read/write support in core#12191mbellade wants to merge 1 commit intohibernate:mainfrom
@Audited entities read/write support in core#12191Conversation
mbellade
force-pushed
the
core-audited
branch
2 times, most recently
from
ef5e845 to
200ed3d
Compare
mbellade
force-pushed
the
core-audited
branch
3 times, most recently
from
f737bd0 to
f0e3c69
Compare
mbellade
force-pushed
the
core-audited
branch
2 times, most recently
from
8dd9368 to
08543a0
Compare
| @GeneratedValue | ||
| @RevisionEntity.TransactionId | ||
| @Column(name = "REV") | ||
| private int id; |
There was a problem hiding this comment.
Maybe long would be more appropriate?
There was a problem hiding this comment.
Sure, I copied the mapping from the existing Envers one and left it as-is for maximum compatibility, but I guess a long mapping should read/write fine to existing schemas anyway, right?
There was a problem hiding this comment.
I'm pretty sure that most JDBC drivers are capable of implicitly converting BIGINT to INTEGER on write and the other way around on read.
mbellade
force-pushed
the
core-audited
branch
2 times, most recently
from
a737470 to
3e24736
Compare
|
|
|
||
| @RevisionEntity.Timestamp | ||
| @Column(name = "REVTSTMP") | ||
| private long timestamp = System.currentTimeMillis(); |
There was a problem hiding this comment.
Can't we use @CreationTimestamp here instead? That way, a user would also have the possibility to influence the clock, if needed during testing.
There was a problem hiding this comment.
Funny thing: I've tried, but it seems like long is not handled by our CurrentTimestampGeneration for the SourceType.VM (in-memory) generation case, and we fallback to the DB but there we get an error for incompatible types (localtimestamp vs BIGINTEGER).
Edit: should I change that while I'm at it, or is System.currentTimeMillis() fine for now? Since the revision entity instance is created just moments before persisting / flushing it, it should be effectively the same.
| public final class AuditHelper { | ||
| public static final String TRANSACTION_ID = "transactionId"; | ||
| public static final String MODIFICATION_TYPE = "modificationType"; | ||
| public static final String TRANSACTION_END = "transactionEnd"; |
There was a problem hiding this comment.
Should probably also align the naming here, no?
| public static final String TRANSACTION_END = "transactionEnd"; | |
| public static final String TRANSACTION_END_ID = "transactionEndId"; |
| if ( data.allRevisions ) { | ||
| data.getRowProcessingState().getSession() | ||
| .getLoadQueryInfluencers() | ||
| .setTemporalIdentifier( ALL_REVISIONS ); |
There was a problem hiding this comment.
Is it possible, that an outer load sets one transaction id, and then this inner load resets this to ALL_REVISIONS, causing problems for subsequent initializers?
I'm thinking about a EntityInitializer containing a EntitySelectFetchInitializer, then containing another EntityInitializer with ALL_REVISIONS. Maybe you should rather track the current temporal identifier where you set it within the Data object and set it back to the previous value here?
There was a problem hiding this comment.
I don't think so, the data.allRevisions flag is only set for the root entity result initializer (the one that has the transactionIdAssembler), so it should only be reset after the entire row is read.
| // Set the per-row temporal identifier so that association loads use the correct revision | ||
| data.getRowProcessingState().getSession() | ||
| .getLoadQueryInfluencers() | ||
| .setTemporalIdentifier( data.entityKey.getTransactionId() ); |
There was a problem hiding this comment.
Is this method also being called for all the other initialization paths, like when the entity comes from the first level cache and is initialized or second level / query cache? My hunch is, that we might also have to do this call in the other resolveInstance methods.
There was a problem hiding this comment.
Are you talking about the resolveInstance(Object, Data)? if that's the one, that is only for associated entities from a root initializer that found an instance already in the PC, but the root initializer itself would have called the resolveInstance(data) version which sets the tx-id.
Also resolveInstanceFromCache as well is called after we go through here, so I think we're fine in all cases.
3 participants
https://hibernate.atlassian.net/browse/HHH-20214
Read and write support for
@Auditedentities modeled afterhibernate-envers:AuditLogAPI, alternative toAuditReaderfor built-in query functionalitytransactionId()andmodificationType()functionsALL_REVISIONSmode for historical queries (fetch all past revisions)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-20214 (Improvement):
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)