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

Allow to detect Entities which no longer match subscription criteria #1517

Merged
merged 10 commits into from
Jun 6, 2023

Conversation

armiol
Copy link
Contributor

@armiol armiol commented Jun 4, 2023

This is a port of #1504 to master. Here is a full description of the PR, for reviewer's convenience.

This changeset adds an ability to subscribe to changes in Entity states, which make them no longer matching the subscription criteria.

In particular, this will always be the case if Entity becomes archived or deleted.

The new endpoint is available for Spine client under whenNoLongerMatching() DSL, and is a part of Client's request API:

    var client = client();
    client
        /* ... */
        .subscribeTo(Task.class)
        .observe((task) -> { /* ... */ })
        .whenNoLongerMatching(TaskId.class, (idOfNonMatchingEntity) -> { /* ... */})
        .post();

These changes will also become available to other clients (such as web-related client), once this PR is merged.

Another thing is a change in how subscriptions work for events, by which type the parent Bounded Context cannot be determined. This is the case if events of some type are not explicitly declared in handler methods (i.e. returned as a plain EventMessage). Prior to this PR, a new subscription for events of such types was created in every Bounded Context known to SubscriptionService. But the cancellation of such subscriptions did not work properly (did nothing!).

Now, symmetrically to creating a Subscription in "no-parent-context-found" use-case (i.e. creating it in every known Bounded Context), such a subscription is cancelled in all known Bounded Contexts as well.

The library version is now set to 2.0.0-SNAPSHOT.147.

@armiol armiol self-assigned this Jun 4, 2023
@sonatype-lift
Copy link

sonatype-lift bot commented Jun 4, 2023

🛠 Lift Auto-fix

Some of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1

# Download the patch
curl https://lift.sonatype.com/api/patch/github.com/SpineEventEngine/core-java/1517.diff -o lift-autofixes.diff

# Apply the patch with git
git apply lift-autofixes.diff

# Review the changes
git diff

Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command:

curl https://lift.sonatype.com/api/patch/github.com/SpineEventEngine/core-java/1517.diff | git apply

Once you're satisfied, commit and push your changes in your project.

Footnotes

  1. You can preview the patch by opening the patch URL in the browser.

armiol added 3 commits June 4, 2023 17:37
…f `Subscription` is known to current Bounded Context.
…te (i.e. creating it in every known Bounded Context), cancel it in all known Bounded Contexts as well.
@armiol armiol changed the base branch from third-party-events-with-tenancy to master June 5, 2023 11:30
@armiol armiol marked this pull request as ready for review June 5, 2023 14:24
@armiol
Copy link
Contributor Author

armiol commented Jun 5, 2023

@alexander-yevsyukov PTAL.

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #1517 (0d4a6e5) into master (1eef060) will increase coverage by 0.17%.
The diff coverage is 87.44%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1517      +/-   ##
============================================
+ Coverage     89.71%   89.88%   +0.17%     
- Complexity     4934     4984      +50     
============================================
  Files           635      644       +9     
  Lines         15454    15618     +164     
  Branches        888      901      +13     
============================================
+ Hits          13864    14039     +175     
+ Misses         1286     1266      -20     
- Partials        304      313       +9     

Copy link
Contributor

@alexander-yevsyukov alexander-yevsyukov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with few minor requests on null-checking

* the consumer to delegate the observation to
*/
NoLongerMatchingConsumer(Class<I> idClass, Consumer<I> delegate) {
this.idClass = idClass;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we check for null input here, please?

private final NoLongerMatchingConsumer<?> consumer;

NoLongerMatchingFilter(NoLongerMatchingConsumer<?> consumer) {
this.consumer = consumer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for null input here too.

* <p>Specifies no chained observer.
*
* @param targetObserver a delegate consuming the {@code Entity} state, or an {@code Event}
*/
SubscriptionObserver(StreamObserver<M> targetObserver) {
this.delegate = targetObserver;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for null input.

*/
SubscriptionObserver(StreamObserver<M> targetObserver,
StreamObserver<SubscriptionUpdate> chain) {
this.delegate = targetObserver;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check for null input.


import com.google.errorprone.annotations.CheckReturnValue;

import javax.annotation.ParametersAreNonnullByDefault;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add trailing empty line.

@armiol
Copy link
Contributor Author

armiol commented Jun 6, 2023

As discussed vocally, we don't generally perform null-checks for the ctor parameters if the API is package-level.

Therefore, comments related to this issue were not addressed.

@armiol armiol added this pull request to the merge queue Jun 6, 2023
Merged via the queue into master with commit 8b2daa8 Jun 6, 2023
@armiol armiol deleted the allow-detecting-entities-stop-matching-subscriptions branch June 6, 2023 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants