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

Permit wildcard in tag validator URIs #858

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

eslavich
Copy link
Contributor

This implements an idea from @CagtayFabry discussed in asdf-format/asdf-standard#271. See #838 for an alternative implementation.

The PR is branched off of #850. The relevant changes are in the latest commit.

The idea is to allow pattern matching in the tag validator, so that schema authors can permit many versions of a tag. For example, the following pattern:

tag: asdf://somewhere.org/tags/foo-*

will match any of the following:

asdf://somewhere.org/tags/foo-1.0.0
asdf://somewhere.org/tags/foo-2.0

and this will match any transform tag:

asdf://asdf-format.org/extensions/transform/tags/*

which I expect will save us a lot of tedious updating of schemas when we add or update transforms.

@eslavich eslavich added this to the 2.8.0 milestone Aug 10, 2020
@eslavich eslavich force-pushed the eslavich-wildcard-match-uris branch 2 times, most recently from 860a027 to 1a33ab1 Compare August 10, 2020 18:15
@eslavich eslavich requested review from jdavies-st, perrygreenfield, nden and kmacdonald-stsci and removed request for nden August 10, 2020 18:25
@CagtayFabry
Copy link
Contributor

Thanks for tagging me @eslavich

I thought a little bit about asdf-format/asdf-standard#271 over last few days but did not find the time to write down everything.
There is one point I would like to discuss regarding the * wildcard implementation (which I really like in this PR):
Ending a tag URI on * will allow very lenient pattern matching even for completely different tags/versions.

consider the following (passing) addition to test_uri_match:

("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-bar/tag-3.1", True),

Of course this might be intended, but what if only all versions of the actual tag asdf://*/tags/foo where supposed to be allowed? Currently I do not see the way to fix the tag but allow all versions. Like this behavior:

("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-4.9", True),
("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-5.2", True),
("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-bar/tag-3.1", False),

I am not sure about the side effects this may or may not have.

To avoid this I would propose that all tag strings must end on a valid version pattern like -*, -1, -1.*, -1.2, -1.2.*, -1.2.3 including the hyphen to be more explicit about the versioning. Basically this would enforce a trailing hyphen to always separate tag URI from versioning. (similar to what I was going for in #838)

So for the above example we could get the following behavior to work (which I personally find more explicit):

("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-4.9", True),
("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-5.2", True),
("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-bar/tag-3.1", False),
("asdf://*/tags/foo*-*", "asdf://anywhere.org/tags/foo-bar/tag-3.1", True),

@eslavich
Copy link
Contributor Author

Of course this might be intended, but what if only all versions of the actual tag asdf://*/tags/foo where supposed to be allowed? Currently I do not see the way to fix the tag but allow all versions.

Hmm... that's a good point. I didn't think about that.

To avoid this I would propose that all tag strings must end on a valid version pattern

I'm only resistant to this because I suspect some people will want to use versions in some other way, maybe in the path ahead of the tag. Currently the ASDF Standard and the YAML specification both permit the tag to be any valid URI, and I'm not sure it's worth restricting them to support pattern matching on our particular notion of what a versioned URI should look like.

Here's another idea: how about we treat * more like a filesystem glob pattern? So * would absorb any character except the / and stop matching if it encounters the /. That interpretation would make this example work:

("asdf://*/tags/foo-*", "asdf://anywhere.org/tags/foo-bar/tag-3.1", False),

We could even use the ** in the globstar way to imply matching any number of nested "directories":

("asdf://*/tags/**/tag-*", "asdf://anywhere.org/tags/foo/tag-3.1", True),
("asdf://*/tags/**/tag-*", "asdf://anywhere.org/tags/bar/tag-4.5", True),

There is still potential for overly lenient matching within the same "directory":

("asdf://anywhere.org/tags/foo-*", "asdf://anywhere.org/tags/foo-3.1", True),
("asdf://anywhere.org/tags/foo-*", "asdf://anywhere.org/tags/foo-bar-4.5", True),

But we can enforce that this does not happen in the ASDF core tags (with an automated test) and in other officially supported ASDF extensions. If you do the same in your project, would this system work? Or is the matching on foo-3.1 and foo-bar-4.5 still problematic?

@CagtayFabry
Copy link
Contributor

Yes, the * and ** glob-like pattern matching the way you described was exactly my idea as well when I thought about this. (I also find it very readable and intuitive in comparison to e.g. regex)

As long as the syntax is well described I don't really see any issues. Sure

("asdf://anywhere.org/tags/foo-*", "asdf://anywhere.org/tags/foo-bar-4.5", True),

might be weird but should be avoidable when aware of the implementation.

@eslavich
Copy link
Contributor Author

Great, I'll adapt this PR to implement the glob behavior. Thanks for helping me think this through!

@eslavich eslavich force-pushed the eslavich-wildcard-match-uris branch from 325f7e9 to 1890063 Compare August 12, 2020 17:12
@eslavich eslavich merged commit 535f958 into asdf-format:master Aug 12, 2020
@eslavich eslavich deleted the eslavich-wildcard-match-uris branch August 12, 2020 17:58
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.

2 participants