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

NotifyingSailConnection should distinguish between explicit and inferred statements #5118

Open
kenwenzel opened this issue Aug 27, 2024 · 5 comments
Labels
📶 enhancement issue is a new feature or improvement

Comments

@kenwenzel
Copy link
Contributor

kenwenzel commented Aug 27, 2024

Problem description

When listening to changes on NotifyingSailConnection the consumer is unable to distinguish between explicit and inferred statements.
In scenarios where undo/redo or audit logging should be implemented it would be very helpful if this distinction could be drawn.

Preferred solution

The interface SailConnectionListener could be extended to include an inferred flag as parameter for the notification methods.
The default implementation of those methods could just delegate to the existing ones.

public interface SailConnectionListener {
  void statementAdded(Statement st);
  void statementRemoved(Statement st);
  
  default void statementAdded(Statement st, boolean inferred) {
    statementAdded(st);
  }

  default void statementRemoved(Statement st, boolean inferred) {
    statementRemoved(st);
  }
}

Are you interested in contributing a solution yourself?

None

Alternatives you've considered

The alternative would be to directly include the inferred flag within the statement interface.
This is the approach that we have used for KOMMA:
https://github.com/komma/komma/blob/master/bundles/core/net.enilink.komma.core/src/main/java/net/enilink/komma/core/IStatement.java

Anything else?

No response

@kenwenzel kenwenzel added the 📶 enhancement issue is a new feature or improvement label Aug 27, 2024
@hmottestad
Copy link
Contributor

Sounds good! We’ll have to be a bit careful that we don’t make any breaking changes.

I think the reason why Statements don’t have an inferred flag is that users can’t insert their own inferred statements. We could introduce an interface StatementWithInferredStatus (or something a bit cleaner) and then we can use instanceOf to check if the underlying store supports that feature (eg. produces statements that implement that interface).

@kenwenzel
Copy link
Contributor Author

As a first step we could just extend the listener as sketched above. This would save users from false assumptions about the support of inferred states.

At a later point:
Instead of StatementWithInferredStatus I would prefer to simply extend the Statement interface with isInferred() and add a default method that just returns false.
Within KOMMA I have decided to not include the inferred flag for equals() and hashCode() but I am not sure if this is a good/correct decision.

@hmottestad
Copy link
Contributor

hmottestad commented Aug 27, 2024

To extend the listener we would have to add two new methods in order to keep it backwards compatible, and both of those methods need a default implementation on the interface side. Those implementations would probably just pass down to the original methods, but for users who implement the methods there would not be a guarantee that the sail actually calls the new methods.

@kenwenzel
Copy link
Contributor Author

Yes, that is right. But I guess - despite of the SAILs already delivered with RDF4J - there is not much use of this interface.
Hence it would be a minimal invasive change.

@hmottestad
Copy link
Contributor

There are some databases that are built on the Sail interfaces, but I don't think this is going to be that big of a deal for those.

Why don't you make a PR and we can see how it looks and functions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📶 enhancement issue is a new feature or improvement
Projects
None yet
Development

No branches or pull requests

2 participants