replace regex with jena request parser #913
Conversation
| * | ||
| * @see <a href="https://jira.duraspace.org/browse/FCREPO-1409"> FCREPO-1409 </a> for details. | ||
| */ | ||
| private Optional<Quad> checkInvalidPredicates(final UpdateRequest request) { |
There was a problem hiding this comment.
Why is this returning/not-returning a Quad?
There was a problem hiding this comment.
Do you mean: why a Quad instead of a predicate String? The only reason it's returning an Optional like this (instead of, e.g., a boolean) is so that the throw expression can include the problematic part of the statement in the message.
There was a problem hiding this comment.
... but from my perspective, it could be a Quad or something else. A Quad is easier, since that's what the source data consists of.
There was a problem hiding this comment.
No, I mean why a Quad or anything else instead of some kind of information about what went wrong. Why is this code not returning some kind of description of a condition or conditions of invalidity?
There was a problem hiding this comment.
I could instead make this function return nothing and push the throw new IllegalArgumentExeption() into this function. Would you find that a cleaner implementation?
There was a problem hiding this comment.
Yes. Another, more mannered approach would be to return Collection<? extends Exception>, which would allow for a more sophisticated report.
|
@ajs6f I replaced the |
|
Because with a |
|
But then it would be better to respond with a |
|
No, an exception represents an error. A string is a message about the error. I'm saying that this method should return errors, not messages. It's up to other parts of the system to convert between the two. You can do other things with an |
|
@ajs6f I pushed a new commit to this PR; is this more along the lines of what you mean? |
| final StringJoiner errorMessages = new StringJoiner(", "); | ||
| for (final IllegalArgumentException ex : errors) { | ||
| errorMessages.add(ex.getMessage()); | ||
| } |
There was a problem hiding this comment.
No loop, just errors.stream().map(Exception::getMessage).collect(joining(",\n"))
|
Resolved with: 6ea28b2 |
See: https://jira.duraspace.org/browse/FCREPO-1706
This removes the (incorrect) use of regular expressions in checking predicate values.