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

dids with trailing "/" stopped working #1393

Closed
bari12 opened this issue Jul 27, 2018 · 2 comments
Closed

dids with trailing "/" stopped working #1393

bari12 opened this issue Jul 27, 2018 · 2 comments

Comments

@bari12
Copy link
Member

bari12 commented Jul 27, 2018

Motivation

Trailing "/" in Rucio are not part of the DID name, however, the historic ATLAS naming did include them, such as: mc16_13TeV:mc16_13TeV.301024.PowhegPythia8EvtGen_AZNLOCTEQ6L1_DYmumu_600M800.deriv.DAOD_EXOT0.e3649_s3126_r9364_r9315_p3374/
For most commands REST endpoints including the DID are used, where this trailing / resulted in a // in the URL. This // is interpreted as one / in httpd and thus the command did still deliver a result.

However, since the 1.16.* clients, both scope and name get escaped using urllib.quote_plus(). This way the name gets transformed in e.g.: mc16_13TeV.301024.PowhegPythia8EvtGen_AZNLOCTEQ6L1_DYmumu_600M800.deriv.DAOD_EXOT0.e3649_s3126_r9364_r9315_p3374%2F and the lookup fails.

Modification

Possible solutions:

  1. Use urllib.quote_plus(s, '/') to declare '/' as a safe character. This should work, but for commands which do not use the did in the REST call, the trailing / of a container was and will still be a problem. E.g.: list_dids with a filter; attach_dids (container to container), detach_dids (container from container) etc. - This also fails for communites using '/' in their naming scheme
  2. Filter trailing / completely in the client. - Might be problematic for communities
  3. Ask external applications to filter trailing / on their side.
@bbockelm
Copy link
Contributor

Beware that %2F is a really tricky character to work with, especially in APIs that include the DID in the URL path itself. For example, mod_wsgi always converts these back to /.

How good is the unit test coverage in this area?

@bari12
Copy link
Member Author

bari12 commented Jul 31, 2018

Test coverage is bad in this. Essentially we never tested with any / in there (not even the trailing one) as for us the container names (in ATLAS) do not include the trailing /. So we need to add some tests with "/" or other special characters in the names.

Solution will be most likely that we ask external applications in ATLAS not to submit the trailing /, since the other solutions impact communities using slashes in their names (which is valid).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants