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

Callbacks refactoring #1891

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

micopiira
Copy link
Contributor

Aim of this PR is to bring the callbacks & events functionality in spring-data-couchbase
on par with other spring data projects, such as:

WARNING: This PR contains a breaking changes:

Before this PR, the AfterConvertCallback was called before
save operations. This is no longer the case as
AfterConvertCallback is now only called after find operations.
Applications currently using AfterConvertCallbacks should
use BeforeSaveCallback instead. This change was done to be
consistent with other spring data projects.

Callbacks called and events published during various operations in order

during Reactive save operations

  • BeforeConvertEvent
  • ReactiveBeforeConvertCallback
  • BeforeSaveEvent
  • ReactiveBeforeSaveCallback
  • save to db
  • AfterSaveEvent
  • ReactiveAfterSaveCallback

during Reactive fetch operations

  • fetch from db
  • AfterConvertEvent
  • ReactiveAfterConvertCallback

during blocking save operations

  • BeforeConvertEvent
  • BeforeConvertCallback
  • BeforeSaveEvent
  • BeforeSaveCallback
  • save to db
  • AfterSaveEvent
  • AfterSaveCallback

during blocking fetch operations

  • fetch from db
  • AfterConvertEvent
  • AfterConvertCallback

TODO:

  • Update reference documentation
  • Bump major version?

Fixes #1854

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

Adds support for modifying entities after find operations
through AfterConvertCallbacks and also
unified the callbacks API with other
Spring data projects such as R2DBC & MongoDB

BREAKING:

Before this change AfterConvertCallback was called before
save operations. This is no longer the case as
AfterConvertCallback is now only called after find operations.
Applications currently using AfterConvertCallbacks should
use BeforeSaveCallback instead.

See:

https://docs.spring.io/spring-data/r2dbc/docs/1.4.4/api/org/springframework/data/r2dbc/mapping/event/package-summary.html

https://docs.spring.io/spring-data/mongodb/docs/current/api/org/springframework/data/mongodb/core/mapping/event/package-summary.html
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 3, 2024
@mikereiche
Copy link
Collaborator

mikereiche commented Jan 11, 2024

Thanks for offering this pull request. Breaking changes can only go in major versions.

If you make a PR with only "To be able to use spring-data-couchbase I would need an easy way to modify entities after fetching them from the database, as currently there is no easy way to do this are you interested in implementing new callbacks for modifying entities after find operations", then I will review that. The callback can be named something like AfterDeserializationCallback so it does not conflict with AfterConvertCallback that currently serves another purpose. Please do not include other changes (such as updating comments or removing classes - that can go in separate PRs).

I appreciate the effort to unify the callback class names. But if the class names are going to be changed, they should be changed to names that make sense. For instance, one could expect the calls

BeforeConvertCallback
-- convert --
AfterConvertCallback
-- save --
AfterSaveCallback

but that is not the case for mongodb. Instead they have

BeforeConvertCallback
-- converter --
BeforeSaveCallback
-- save --
AfterSaveCallback

Which is confusing because both callbacks are before the save. As far as I can tell, r2dbc was copied from mongodb resulting in the same problem.

If we're going to the trouble of renaming, why not BeforeSerializationCallback, AfterSerializationCallback, AfterSaveCallback on write, and then on read BeforeDeserializationCallback and AfterDeserializationCallback on read()? Or better yet - BeforeSerializationOnReadCallback etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReactiveAfterConvertCallback not called after find operations
3 participants