Repository navigation
Proposal: More flexible CommitPropertiesProvider #1240
Description
Activity
@orange-guo I like the idea of getting rid of ThreadLocal but the proposed solution it's not clear for me. How do you pass props? Could you provide more snippets to cover the full use case?
Firstly, I would like to make some additions to the previous design. At the beginning of this proposal, I suggested adding an
auditedMethodparameter. However, now I think we should not limit ourselves to just passingauditedMethodbut instead pass anAuditingContext, it's an interface that provides the ability to read contextual information when the auditing action occurs, such as method, class, and other information. This approach will make it easier for us to extend in the future. Here is an example.public interface AuditingContext { public Method getMethod(); public Class<?> getClass(); // more additional method }The following content is about the parameter passing you just mentioned. After reviewing the source code, I found that the current call to
CommitPropertiesProviderto obtaincommitPropertiesoccurs inorg.javers.spring.auditable.aspect.JaversCommitAdvice. This means that when calling, we need to create anAuditingContext. Currently, theAuditingContextcontains information such as method and class, which can be provided by theJoinPointpassed whenJaversCommitAdviceis called byJaversAuditableAspectAsync,JaversAuditableAspect, andJaversSpringDataAuditableRepositoryAspectHere is the snippets
JaversAuditableAspectAsync.java
@AfterReturning("@annotation(org.javers.spring.annotation.JaversAuditableAsync)") public void commitAdvice(JoinPoint pjp) { lastAsyncCommit = javersCommitAdvice.commitSaveMethodArgumentsAsync(pjp); }JaversCommitAdvice.java
Optional<CompletableFuture<Commit>> commitSaveMethodArgumentsAsync(JoinPoint pjp) { List<CompletableFuture<Commit>> futures = AspectUtil.collectArguments(pjp) .stream() .map(arg -> commitObjectAsync(arg)) // 1. we need to pass the pjp to commitObjectAsync .collect(Collectors.toList()); return futures.size() == 0 ? Optional.empty() : Optional.of(futures.get(futures.size() - 1)); } CompletableFuture<Commit> commitObjectAsync(Object domainObject) { String author = this.authorProvider.provide(); // 2. we need to pass the pjp to propsForCommit return this.javers.commitAsync(author, domainObject, propsForCommit(domainObject), executor); } private Map<String, String> propsForCommit(Object domainObject) { // 3. We need to create the AuditingContext here, for performance we need to include the caching solution return Maps.merge( commitPropertiesProvider.provideForCommittedObject(domainObject), commitPropertiesProvider.provide()); }Finally, based on my current understanding, the scope of impact mainly includes the following:
- Adding
AuditingContext - Adding a default implementation of AudiitngContext
- Adding AuditingContextFactory responsible for creating AuditingContext
- JaversCommitAdvice needs to add a field to include AuditingContextFactory
- Adding parameters to CommitPropertiesProvider methods to ensure that AuditingContext can be passed
- Refactoring JaversCommitAdvice to create AuditingContext using AuditingContextFactory and pass it to - CommitPropertiesProvider when calling commitPropertiesProvider.
- Adding
But when you have a Method, you can easily get its class, by getDeclaringClass(), so the concept of AuditingContext seems redundant to me for now. I suggest to simplify this design and stick to bare Method, as auditing contect
My example may not be clear enough. Please consider the following situation: if we need to provide users with the parameters of the audited method at runtime in the future, we need to extend the existing parameter list. The advantage of using AuditingContext is that subsequent extensions will not affect user usage (it will not cause parameter list changes and cause compilation problems).
Hi @orange-guo , sorry for slow response. Okay , I appreciate the idea of AuditingContext. It can't be simply added to
CommitPropertiesProviderwithout braking backward compatibility. So I guess a new bean type could be introduced, likeAdvancedCommitPropertiesProvider.JaversCommitAdviceshould support both beansHi @bartoszwalacik, It's nothing. I agree the new design is better than before modifying the
JaversCommitAdvice.Changes
If we decide to introduce it, we need to make the following changes:
Code Implementation
- Add the interface
AdvancedCommitPropertiesProvider. - Add the interface
AuditingContextand its implementation. - Inject
AdvancedCommitPropertiesProviderintoJaversCommitAdvicevia Spring DI, and call it
Docs
- Add the introduction of
AdvancedCommitPropertiesProviderinto the documentation (usage).
Mark
CommitPropertiesProvideras deprecated?Should we consider marking it as deprecated (maybe deprecated is a better option)?
- Add the
@Deprecatedannotation onJaversCommitAdviceand add reason of deprecated.
These are my estimated changes. Please review them. If needed, I can request a PR to complete it.
- Add the interface
Sounds good! Just a small correction : Add the @deprecated annotation on CommitPropertiesProvider and add reason of deprecated. If you contribute a PR I will merge it promptly. Thanks
- added a commit that references this issue
on Jun 13, 2024 Thanks for your contribution @orange-guo . I decided to change it a bit in order to reduce the impact on existing client's code. So the original CommitPropertiesProvider will stay untouched and won't be deprecated, the new interface (AdvancedCommitPropertiesProvider) extends the old one. So it's a kind of optional extension. I know that this approach (one interface extends another) is a bit unusual , but thanks to that we won't force client to change their code
PTAL:
https://github.com/javers/javers/pull/1390/filesReacted by theorangeguo@bartoszwalacik I have no problem with this change, it is the right decision to avoid forcing users to modify their code.
Reacted by Bartosz Walacik- added 3 commits that reference this issue
on Jun 23, 2024 This new feature will be released in 7.5.0, see Javadoc in https://github.com/javers/javers/blob/master/javers-spring/src/main/java/org/javers/spring/auditable/AdvancedCommitPropertiesProvider.java
- added 3 commits that reference this issue
on Jun 23, 2024
Proposal: More flexible CommitPropertiesProvider
Context
Some cases need to generate commit Properties through the audited method.
The current solution is to extend Aspect to put the currently executed method information into ThreadLocal, but this is not a good practice.
Decision
auditedMethod)Add parameter for the following methods of CommitPropertiesProvider
Get auditedMethod from JoinPoint at JaversCommitAdvice call
Example
(pjp.signature as MethodSignature).method)Consequences
After the client code is upgraded, it needs to be changed. It is best to migrate smoothly.
In order to ensure a smooth transition to the new calling method, we need to keep the old method and mark it as depredicate and wait for the old method to be deleted in subsequent versions