Fix & enhancements to the Events API hierarchy #2629
Conversation
|
cc @adnanhemani |
| * Event details are documented under the event objects themselves. | ||
| */ | ||
| public abstract class PolarisEventListener { | ||
| public interface PolarisEventListener { |
There was a problem hiding this comment.
This was extensively debated at the time, but see this comment and related ones for why this is an ABC rather than an interface.
There was a problem hiding this comment.
Thanks for flagging it, @eric-maynard. I still hold my point, that this should be ABC, not interface.
There was a problem hiding this comment.
Thank you both for reviving this discussion. I've re-read the old thread carefully, and I still do not understand what the problem is with using an interface rather than an abstract class.
Can you please summarize your concerns here for clarity?
If it's about binary compatibility when adding more methods to the API, I'm afraid an interface and an ABC provide roughly the same linkage risks. From JLS 13.4.16 (emphasis mine):
adding a default method is a binary-compatible change because it does not introduce errors at link time, even if it introduces errors at compile time or invocation time. In practice, the risk of accidental clashes occurring by introducing a default method are similar to those associated with adding a new method to a non-final class.
In contrast, the advantages of using interfaces rather than abstract classes in public APIs are abundantly documented. From Effective Java, 3rd Edition (J. Bloch, 2017) (emphasis mine):
Item 20: Prefer interfaces to abstract classes
Java has two mechanisms to define a type that permits multiple implementations: interfaces and abstract classes. Since the introduction of default methods for interfaces in Java 8 [JLS 9.4.3], both mechanisms allow you to provide implementations for some instance methods. A major difference is that to implement the type defined by an abstract class, a class must be a subclass of the abstract class. Because Java permits only single inheritance, this restriction on abstract classes severely constrains their use as type definitions. Any class that defines all the required methods and obeys the general contract is permitted to implement an interface, regardless of where the class resides in the class hierarchy.
There was a problem hiding this comment.
Yes, binary compatibility is the concern. The upstream changes shouldn't break downstream event listeners. However, looking over the PR, it looks like all methods are now default methods, which is fine. Let's also ensure that any new methods added in the future are default as well.
Could we add a test to enforce this? It might also be helpful to include a comment on the class to explicitly state that all new methods should be default.
There was a problem hiding this comment.
Hi @flyrain thanks for your flexibility 🙂
I will add the comment and the test as suggested.
I'd note that this kind of check would be a good fit for the revapi plugin – the same one we use in Iceberg to track API breaking changes. But I think Polaris is still a young project, and revapi would flag too many API changes that would be otherwise permitted by our current evolution rules:
polaris/site/content/in-dev/unreleased/evolution.md
Lines 81 to 82 in b1eb9bd
So, let's stick with a small test for now 👍
There was a problem hiding this comment.
Added a comment and tests as suggested.
I also re-organized the interface methods into "sections" for clarity, I hope that's OK.
The tests check that all methods are default and well-formed, and also that all events have corresponding methods in the interface.
I added a similar check to TestPolarisEventListener (it was lacking many methods).
Summary of changes: - Turned `PolarisEventListener` into an interface to facilitate implementation / mocking - Added missing `implements PolarisEvent` to many event records - Removed unused method overrides - Added missing method overrides to `TestPolarisEventListener`
adutra
force-pushed
the
event-listener-interface
branch
from
8f674f4 to
1c50978
Compare
| @Override | ||
| public Response listCatalogs(RealmContext realmContext, SecurityContext securityContext) { | ||
| polarisEventListener.onBeforeListCatalog(new CatalogsServiceEvents.BeforeListCatalogEvent()); | ||
| polarisEventListener.onBeforeListCatalogs(new CatalogsServiceEvents.BeforeListCatalogsEvent()); |
There was a problem hiding this comment.
When I added tests for method compliance I noticed that this event was different from other "list" events because the object type being listed wasn't in the plural.
|
@flyrain @eric-maynard is the code looking better now? |
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
3 participants
Summary of changes:
PolarisEventListenerinto an interface to facilitate implementation / mockingimplements PolarisEventto many event recordsTestPolarisEventListener