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

feat(Event Framework): add intermediate superclass for Asset Events #1815

Conversation

janpmeyer
Copy link
Contributor

What this PR changes/adds

Add a 'intermediate' superclass for the Event Classes AssetCreated and AssetDeleted.

Why it does that

Improve the work with the Event Framework and give the possibility to filter Events on a bigger group of events

Linked Issue(s)

Closes #1813

Checklist

  • added appropriate tests?
  • performed checkstyle check locally?
  • added/updated copyright headers?
  • documented public classes/methods?
  • added/updated relevant documentation?
  • assigned appropriate label? (exclude from changelog with label no-changelog)
  • formatted title correctly?

@codecov-commenter
Copy link

codecov-commenter commented Aug 10, 2022

Codecov Report

Merging #1815 (34c29eb) into main (8bf9379) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1815      +/-   ##
============================================
- Coverage     62.54%   62.53%   -0.01%     
- Complexity        0     3182    +3182     
============================================
  Files           781      782       +1     
  Lines         16592    16592              
  Branches       1079     1079              
============================================
- Hits          10377    10376       -1     
- Misses         5767     5768       +1     
  Partials        448      448              
Impacted Files Coverage Δ
...taspaceconnector/spi/event/asset/AssetCreated.java 100.00% <100.00%> (ø)
...taspaceconnector/spi/event/asset/AssetDeleted.java 100.00% <100.00%> (ø)
...ceconnector/spi/event/asset/AssetEventPayload.java 100.00% <100.00%> (ø)
...iation/ProviderContractNegotiationManagerImpl.java 90.00% <0.00%> (-0.56%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.


import java.util.Objects;

/**
* Describe an Asset deletion, after this has emitted, the Asset represented by the id won't be available anymore.
*/
public class AssetDeleted extends Event<AssetDeleted.Payload> {
public class AssetDeleted extends AssetEvent {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public class AssetDeleted extends AssetEvent {
public class AssetDeleted extends Event<AssetDeleted.Payload> {

I think the AssetDeleted should be related to its own payload, currently it's the same for both the events, but this could change in any moment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If i change it like the way you suggest their is no superclass again.

But you are right i messed up the Relation. I want sth. like EventPayload <- AssetPayload <- AssetCreatedPayload/AssetDeleted Payload. I need to change it this way.

Copy link
Member

Choose a reason for hiding this comment

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

Looking it from another perspective seems like the hierarchy is more related to the payload than to the event class itself...
Maybe the only class we need to introduce is a AssetEventPayload that contains the assetId and that it could be used to filter out events.


private AssetDeleted() {
}

public static class Builder extends Event.Builder<AssetDeleted, Payload, Builder> {
public static class Builder extends Event.Builder<AssetDeleted, AssetEvent.Payload, Builder> {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, and at a this point I would move the builder logic that permits to add an assetId on the AssetEvent superclass

@janpmeyer janpmeyer force-pushed the feature/event-framework-add-Asset-Event branch 2 times, most recently from 20fe780 to 8800ccb Compare August 23, 2022 09:58
@janpmeyer janpmeyer force-pushed the feature/event-framework-add-Asset-Event branch from 8800ccb to b4f6055 Compare August 23, 2022 10:17
@janpmeyer
Copy link
Contributor Author

The hierarchy is now binded on the Payload classes.

@janpmeyer janpmeyer marked this pull request as ready for review August 23, 2022 10:38
@janpmeyer janpmeyer requested a review from ndr-brt August 23, 2022 10:39
Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

As I said in the last comment, the setAssetId method shouldn't exist

@janpmeyer
Copy link
Contributor Author

Test, who fails has no connection to Eventsystem and touched Files.

Copy link
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

It was a flaky test

@ndr-brt ndr-brt merged commit a9ef4f1 into eclipse-edc:main Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core feature enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Event Framework: Adding "intermediate" superclass for Asset Events
4 participants