gouttegd
commented
1 year ago This is actually not quite right in general, as it does not catch cases like PRO, or OBA, correctly, where (\d+) is not enough - it should be ([A-Z0-9]+) instead.
The pattern is correct for what the OBO specification calls Canonical Prefixed ID, where only digits are allowed in the “local” part.
The pattern will indeed not recognise Non-Canonical Prefixed ID, where almost everything is allowed apart from whitespaces (not just [A-Z0-9]+). I assume this is what is intended, so will leave that pattern as it is.
jamesaoverton
The
OboUtilities.isOboIdmethod inorg.protege.editor.owl.model.utilis currently using thePattern.findmethod to check whether its argument is an OBO ID.This is inappropriate as the
Pattern.findmethod looks for a subsequence within the string, instead of attempting to match the full string. As a result, a bogus OBO ID such as "MONDO:0018299-obsoleted" will be incorrectly identified as a valid OBO ID because thePattern.findmethod will simply stop matching at the first character that does not fit the pattern (the '-'), but will still report that it has find a match.Here, we need to check that the entire string matches the OBO ID pattern, which is why this commit replaces the use of
Pattern.findbyPattern.match, which does precisely that.This is also what the
getOboLibraryIriFromOboIdmethod, in the same class, is doing. For consistency,isOboIdshould do the same, so that whenisOboIdreturns true, the calling code can then invokegetOboLibraryIriFromOboIdon the same string and be confident that the call would not throw a "Invalid OBO Id" RuntimeException.