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

Introduce ValidatingEntityCallback and deprecate ValidatingMongoEventListener #4901

Closed
rfelgent opened this issue Feb 19, 2025 · 7 comments
Closed
Assignees
Labels
status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement

Comments

@rfelgent
Copy link
Contributor

rfelgent commented Feb 19, 2025

Hello,

I would like to contribute a ValidatingEntityCallback (in addition to the ValidatingMongoEventListener) to the code base.

The ValidatingMongoEventListener is not suitable for me due to the design nature of EventListener and EntityCallback. I prefer EntityCallback

IMHO the ValidatingMongoEventListener should (even) be marked as deprecated, because

  • the EntityCallbacks are recommended by the Spring docs and
  • EventListener are by design triggered/executed before any EntityCallback. This can lead to undesired behaviour difficult to track down by a developer, e.g. validation of entity happens before the entity in request is manipulated by any EntityCallback

If the maintainers/team accept my enhancement proposal, I can make a pull request.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 19, 2025
@mp911de
Copy link
Member

mp911de commented Feb 20, 2025

Looking at our Bean Validation integration, MongoDB is the only module that ships with a Bean Validation integration. It is an extremely thin layer component that mandates a dependency on Bean Validation. ValidatingMongoEventListener is registered by our config when Bean Validation is on the classpath.

For the upcoming major 5.0 version of Spring Data MongoDB, we could switch from registering a listerner into registering an EntityCallback. Let me take this ticket to the team to see what they think about it.

@mp911de mp911de added for: team-attention An issue we need to discuss as a team to make progress status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-attention An issue we need to discuss as a team to make progress labels Feb 20, 2025
@mp911de mp911de changed the title [enhancement] add ValidatingEntityCallback (in co-existence to ValidatingMongoEventListener ) Introduce ValidatingEntityCallback and deprecate ValidatingMongoEventListener Feb 24, 2025
@mp911de
Copy link
Member

mp911de commented Feb 24, 2025

After talking with the team, we'd happily accept a pull request.

@rfelgent
Copy link
Contributor Author

rfelgent commented Feb 27, 2025

Cool @mp911de!

One quick question regarding the implementation:

Do you prefer a dedicated "callback-entity" class (not exist) and a dedicated "event-listener" class (already exisists) or is one validator class impementing both interfaces sufficent?

If you prefer the "dedicated" classes option, I tend to put the validation logic in one abstract/base class and let the other classes extend from it.

What do you think ?

@rfelgent
Copy link
Contributor Author

rfelgent commented Mar 1, 2025

Answering the question myself: due to different @Order requirements for ValidatingEntityCallback and ValidatingMongoEventListener I have to design two dedicated classes.

here is the PR

rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 1, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 2, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 2, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 2, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 2, 2025
@mp911de mp911de self-assigned this Mar 3, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 3, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 3, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 3, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 3, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 3, 2025
rfelgent added a commit to gvl-mbh/spring-data-mongodb that referenced this issue Mar 3, 2025
@mp911de mp911de closed this as completed in 54c7d8f Mar 4, 2025
mp911de added a commit that referenced this issue Mar 4, 2025
Introduce ReactiveValidatingEntityCallback, extract BeanValidationDelegate. Document Bean Validation callbacks.

See #4901
Original pull request: #4910
@mp911de mp911de added this to the 4.5 M2 (2025.0.0) milestone Mar 4, 2025
@rfelgent
Copy link
Contributor Author

rfelgent commented Mar 4, 2025

Hi @mp911de ,

thx for finishing the MR !

To my defend, I was not aware that you are keen into that new feature! I had plans for the weekend, now you destroyed my coding plans for the weekend :-P

@mp911de
Copy link
Member

mp911de commented Mar 5, 2025

There is nothing to defend here 😆, I figured that it makes sense to have this change for the next milestone and seeing your commits made me think that you're done. Sorry for crashing your weekend plans, but hey, maybe you want to spend your weekend with #4873 or #4606 if you like.

@rfelgent
Copy link
Contributor Author

rfelgent commented Mar 6, 2025

maybe you want to spend your weekend with #4873 or #4606 if you like

why not. Pls let me hava a look into #4873 . It occurs to me less complex than #4606

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ideal-for-contribution An issue that a contributor can help us with type: enhancement A general enhancement
Projects
None yet
3 participants