Feat/shacl bnodes #1213
Feat/shacl bnodes #1213
Conversation
pj-cegeka
requested review from
Yalz,
rorlic,
Tomvbe and
jobulcke
as code owners
| } | ||
| @ParameterizedTest |
There was a problem hiding this comment.
Can you format this properly? Now no space between the two methods is provided
| } | ||
| } | ||
| static class CorrectMemberArgumentsProvider implements ArgumentsProvider { |
There was a problem hiding this comment.
Can this be formatted as well?
| private static Model readModelFromFile(String fileName) throws URISyntaxException { | ||
| ClassLoader classLoader = IncorrectMemberArgumentsProvider.class.getClassLoader(); | ||
| String uri = Objects.requireNonNull(classLoader.getResource(fileName)).toURI() | ||
| .toString(); | ||
| return RDFDataMgr.loadModel(uri); | ||
| } | ||
|
|
||
| private static Model readModelFromFileCorrectMember(String fileName) throws URISyntaxException { | ||
| ClassLoader classLoader = CorrectMemberArgumentsProvider.class.getClassLoader(); | ||
| String uri = Objects.requireNonNull(classLoader.getResource(fileName)).toURI() | ||
| .toString(); | ||
| return RDFDataMgr.loadModel(uri); | ||
| } |
There was a problem hiding this comment.
Is there a reason why two almost the same methods are provided, but only a different classloader used? Because I think the class loader of the test class itself would work as well. In addition, I think RDFDataMgr.loadModel(fileName) would work fine as well
There was a problem hiding this comment.
I have the same remarks here as with the NodesIngestValidatorTest about formatting and the readModel methods
| verify(memberIngester, times(1)).ingest(anyString(), any(Model.class)); | ||
| .andExpect(status().isBadRequest()) | ||
| .andExpect(content().string(containsString("Member ingested on collection mobility-hindrances should contain the timestamp path: http://www.w3.org/ns/prov#generatedAtTime exactly once."))); | ||
| verify(memberIngester, never()).ingest(anyString(), any(Model.class)); |
There was a problem hiding this comment.
Are you expecting other interactions with this mock? Otherwise, I would propose to use verifyNoInteractions()
| } | ||
|
|
||
| private void validateModel(Model model, EventStream eventStream) { | ||
| Set<Resource> namedSubjects = model.listStatements().mapWith(Statement::getSubject).filterDrop(RDFNode::isAnon).toSet(); |
There was a problem hiding this comment.
Could the listStatements().mapWith(..) be replaced with the listSubjects()
| message.append(eventStream.getCollection()); | ||
| message.append(" should "); | ||
| if (eventStream.isVersionCreationEnabled()) { | ||
| message.append("not "); | ||
| } | ||
| message.append("contain "); | ||
| if (!conformsTimePath) { | ||
| message.append("the timestamp path: "); | ||
| message.append(eventStream.getTimestampPath()); | ||
| } | ||
| if (!(conformsTimePath || conformsVersionPath)) { | ||
| message.append(" and "); | ||
| } | ||
| if (!conformsVersionPath) { | ||
| message.append("the version of path: "); | ||
| message.append(eventStream.getVersionOfPath()); | ||
| } | ||
| message.append(" exactly once."); |
There was a problem hiding this comment.
.append() can the chained together, so I would propose to chainse all the appends which are directly above/below each other, which would result in less bulky code (in my opinion)
| private final String TIMESTAMP_PATH = "http://purl.org/dc/terms/created"; | ||
| private final String VERSIONOF_PATH = "http://purl.org/dc/terms/isVersionOf"; |
There was a problem hiding this comment.
Can you make these variables static please?
| boolean conformsTimePath = model.listStatements(memberSubject, ResourceFactory.createProperty(eventStream.getTimestampPath()), (RDFNode) null).toList().size() == 1 ^ eventStream.isVersionCreationEnabled(); | ||
| boolean conformsVersionPath = model.listStatements(memberSubject, ResourceFactory.createProperty(eventStream.getVersionOfPath()), (RDFNode) null).toList().size() == 1 ^ eventStream.isVersionCreationEnabled(); |
There was a problem hiding this comment.
This is not completely correct: namely when a member contains two versionOfPaths when version creation is enabled, the conforms boolean would be true when expecting false
| @ParameterizedTest | ||
| @ArgumentsSource(IncorrectMemberArgumentsProvider.class) | ||
| void when_IncorrectMemberReceived_Then_ValidationThrowsException(Model model, String collectionName) { | ||
| assertThrows(IngestValidationException.class, () -> validator.validate(model, collectionName)); |
There was a problem hiding this comment.
Can the error message be tested as well, to verify what the validation violation is? This could have detected the mentioned bug above
There was a problem hiding this comment.
Sorry, I forgot a remark, (I got distracting while reviewing this file), but I do not see the need for using logging when the bean is created (but this could be very valuable when you have something like @ConditionalOn(), which is not the case here). Also, the @Order() annotation works fine as well at the IngestValidator implemenations, so could you drop this class and at the @Component and @Order to the implementation classes?
pj-cegeka
force-pushed
the
feat/shacl-bnodes
branch
from
afe420b
to
865044e
Compare
| private final String message; | ||
|
|
||
| public IngestValidationException(String message) { | ||
| this.message = message; | ||
| } | ||
|
|
||
| @Override | ||
| public String getMessage() { | ||
| return message; | ||
| } |
There was a problem hiding this comment.
Maybe I should have noticed this earlier, but can you drop the message propery and use super(message) instead?
| @Component | ||
| @Order(2) | ||
| public class MemberIngestValidator implements IngestValidator { | ||
| private final List<EventStream> eventstreams = new ArrayList<>(); |
There was a problem hiding this comment.
Can this be a set please, as indices are not used and elements are searched by their id/collectionName. Also, as event stream is unique by their id/collectionName, there can be no duplicates present in a set.
| Dataset dataset = RDFParser.source(inputMessage.getBody()).context(rdfModelConverter.getContext()).lang(lang).toDataset(); | ||
| if (dataset.listModelNames().hasNext()) { | ||
| throw new IngestValidationException("Member can not contain named graphs"); | ||
| } | ||
| return dataset.getDefaultModel(); |
There was a problem hiding this comment.
I am not a big fan of performing validation here, but I don't see where else it could happen (or the request body should be converted to a dataset instead of a model), so I am going to leave it like this for now
|
No description provided.