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

GH-4622 SHACL Validation Report preserve blank node IDs with remote repositories #4624

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

Conversation

hmottestad
Copy link
Contributor

@hmottestad hmottestad commented Jun 8, 2023

GitHub issue resolved: #4622

Briefly describe the changes proposed in this PR:

  • switch to RDF4J Binary RDF since this format doesn't rename blank node IDs
  • add parser config option to preserve blank node IDs
  • add test

PR Author Checklist (see the contributor guidelines for more details):

  • my pull request is self-contained
  • I've added tests for the changes I made
  • I've applied code formatting (you can use mvn process-resources to format from the command line)
  • I've squashed my commits where necessary
  • every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

Copy link
Contributor

@abrokenjester abrokenjester left a comment

Choose a reason for hiding this comment

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

It all looks technically correct to me, but I'm puzzled by why you'd need blank node preservation here in the first place.

It's normally something that we only use in very specific internal client-server comms use cases (e.g. in supporting remote transactions). I don't think we ever use it (or recommend using it) for somehow allowing a user to reference a bnode by id remotely.

Also: we tend to use RDF Binary everywhere else to support this: why choose RDF/JSON in this case?

@hmottestad
Copy link
Contributor Author

It all looks technically correct to me, but I'm puzzled by why you'd need blank node preservation here in the first place.

It's normally something that we only use in very specific internal client-server comms use cases (e.g. in supporting remote transactions). I don't think we ever use it (or recommend using it) for somehow allowing a user to reference a bnode by id remotely.

Also: we tend to use RDF Binary everywhere else to support this: why choose RDF/JSON in this case?

I haven't tried using RDF Binary. I'll test it out.

The reason I need to preserve blank node identifiers is that this is a transfer of the validation exception from the server to the client. The user would then typically want to find out more about what went wrong by retrieving the full shape from the server. It's common practice to use blank nodes for PropertyShape(s), so that's why we need to preserve the blank node identifiers.

@hmottestad
Copy link
Contributor Author

TODO

  • Add test to make sure the client still works with the old server

@hmottestad hmottestad force-pushed the GH-4622-shacl-preserve-bnode branch from 938a327 to 8865893 Compare June 17, 2023 06:17
@hmottestad
Copy link
Contributor Author

I think that this should be split into two branches. One with a simple fix that just changes out the format to binary, and a second that changes things up a bit by using the http body. The second branch should also bump the protocol version and target the develop branch.

@hmottestad hmottestad force-pushed the GH-4622-shacl-preserve-bnode branch from 8865893 to 5d652ee Compare October 3, 2023 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SHACL - Blank node IDs should be preserved for remote validation reports
2 participants