linkml / linkml-runtime

Runtime support for linkml generated models
https://linkml.io/linkml/
Creative Commons Zero v1.0 Universal
22 stars 21 forks source link

fix: avoid uri shadowing #280

Closed Silvanoc closed 9 months ago

Silvanoc commented 9 months ago

URIorCURIE validation function is wrongly identifying as CURIEs certain URIs. This patch first tries to validate if a URIorCURIE is a valid URI, otherwise it tries to validate if it's a valid CURIE.

Fixes linkml/linkml#1662

Silvanoc commented 9 months ago

Following invalid URIs or CURIEs which weren't catched by the previous validation are not allowed anymore:

Following invalid URIs which weren't catched by the previous validation are not allowed anymore:

Furthermore following valid URIs that were being rejected are allowed:

[^1]: Support for Safe-CURIEs, which might have '[' and ']' only surrounding the whole CURIE will be added on a later PR

Silvanoc commented 9 months ago

Assuming the support for empty URIs (same-document reference) makes any difficulties, I don't think that deviating from the standard would harm much there.

Silvanoc commented 9 months ago

@cthoyt the module providing validation functions here goes in the direction of what I was wishing to see in the curies package. You might be interested on it or even review it.

It doesn't resolve the URI - CURIE ambiguity where both are accepted yet though.

Silvanoc commented 9 months ago

Small hint for easy manual testing:

poetry run python -c 'from linkml_runtime.utils.uri_validator import validate_uri ; \
    print(validate_uri("urn:namespace:resource"))'

will return a regex match object, since it succeeds.

Silvanoc commented 9 months ago

I've added named groups for better unterstanding of how the validator is parsing the passed strings.

poetry run python -c 'from linkml_runtime.utils.uri_validator import validate_uri ; \
    from pprint import pprint ; \
    pprint(validate_uri("ldap://[2001:db8::7]/c=GB?objectClass?one").groupdict())'

returns

{'authority': '[2001:db8::7]',
 'fragment': None,
 'hier_part': '//[2001:db8::7]/c=GB',
 'host': '[2001:db8::7]',
 'port': None,
 'query': 'objectClass?one',
 'scheme': 'ldap',
 'uri': 'ldap://[2001:db8::7]/c=GB?objectClass?one',
 'userinfo': None}
codecov[bot] commented 9 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (f25abe8) 61.81% compared to head (b86c1fa) 62.11%. Report is 3 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #280 +/- ## ========================================== + Coverage 61.81% 62.11% +0.29% ========================================== Files 62 63 +1 Lines 8393 8459 +66 Branches 2164 2169 +5 ========================================== + Hits 5188 5254 +66 Misses 2599 2599 Partials 606 606 ``` | [Files](https://app.codecov.io/gh/linkml/linkml-runtime/pull/280?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml) | Coverage Δ | | |---|---|---| | [linkml\_runtime/utils/metamodelcore.py](https://app.codecov.io/gh/linkml/linkml-runtime/pull/280?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml#diff-bGlua21sX3J1bnRpbWUvdXRpbHMvbWV0YW1vZGVsY29yZS5weQ==) | `78.43% <100.00%> (+1.06%)` | :arrow_up: | | [linkml\_runtime/utils/uri\_validator.py](https://app.codecov.io/gh/linkml/linkml-runtime/pull/280?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=linkml#diff-bGlua21sX3J1bnRpbWUvdXRpbHMvdXJpX3ZhbGlkYXRvci5weQ==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Silvanoc commented 9 months ago

@sierra-moxon are you taking care of assigning the PR or are you expecting me to pick someone?

sierra-moxon commented 9 months ago

Thanks @Silvanoc! I reviewed and merged; this is terrific! please feel free to assign one of us as a reviewer if needed. :). I also upped your permissions on the repo so you should be able to branch instead of fork if that is easier.

Silvanoc commented 9 months ago

feel free to assign one of us

I'm not sure I know exactly who do you mean by "us". I probably have a rough idea, but I cannot find any place stating something like "this is the LinkML" team or "these are the maintainers".

cthoyt commented 9 months ago

@Silvanoc I'm glad you found a place for this!