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

Resolver: Strip :SDF_FORMAT_ARGS.* from URIs #283

Closed
martastain opened this issue Jul 19, 2024 · 11 comments · Fixed by #285
Closed

Resolver: Strip :SDF_FORMAT_ARGS.* from URIs #283

martastain opened this issue Jul 19, 2024 · 11 comments · Fixed by #285
Assignees
Labels
type: bug Something isn't working

Comments

@martastain
Copy link
Member

Story

usually the resolver should get a URI like this:

ayon+entity://Usd_Base//trees?product=usdtree_42&version=v001&representation=usd

but in some cases it gets the uri like this

ayon+entity://Usd_Base//trees?product=usdtree_42&version=v001&representation=usd:SDF_FORMAT_ARGS:order=100

The SDF_FORMAT_ARGS should not be there but a few function calls slipped by Pixar and now resolver is called with it.

Expected behavior

/api/resolve should ignore :SDF_FORMAT_ARGS

@martastain martastain added the type: bug Something isn't working label Jul 19, 2024
@martastain martastain self-assigned this Jul 19, 2024
@Lypsolon
Copy link

:SDF_FORMAT_ARGS.[^:]*$

@martastain
Copy link
Member Author

:SDF_FORMAT_ARGS.[^:]*$

Why do you skip semicolons after the first match character? Shouldn't :SDF_FORMAT_ARGS.*$ just do the trick?

@BigRoy
Copy link
Contributor

BigRoy commented Jul 19, 2024

Wait a second - shouldn't this actually be caught int he AYON USD resolver itself? I mean - we do have the control to strip it there. Likely I missed discussion but why does this also need to be on the backend?

@martastain martastain linked a pull request Jul 19, 2024 that will close this issue
@martastain
Copy link
Member Author

I guess the main reason is that now it triggers 500 error if it went thru :)

@martastain
Copy link
Member Author

That brings a quesiton: if a URI is malformed, should i abrot completely, or just skip it (so the result set won't contain that uri?)

@Lypsolon
Copy link

:SDF_FORMAT_ARGS.[^:]*$

Why do you skip semicolons after the first match character? Shouldn't :SDF_FORMAT_ARGS.*$ just do the trick?

yes, we can do this. i belive this regx might be better for the task

@Lypsolon
Copy link

Wait a second - shouldn't this actually be caught int he AYON USD resolver itself? I mean - we do have the control to strip it there. Likely I missed discussion but why does this also need to be on the backend?

@dee-ynput wants to have it as dynamic as possible. we do have the option to do it in the resolver, but this would cause recompile, and this would mean all artist stations need to redownload the resolver for the update.

@Lypsolon
Copy link

That brings a quesiton: if a URI is malformed, should i abrot completely, or just skip it (so the result set won't contain that uri?)

currently, it's Http500 and that is very easy to handle in the AyonCppApi, so I always prefer if there is an error code than an empty response because I would need to check the response in that case.

@martastain
Copy link
Member Author

Actually before this issue was identified it never occurred to me. I think the correct response would be 400 as it is a problem with the request. Server should handle it as such and tell the client there's a problem with the request payload. 500 implies server error. which is not the case - or - it shouldn't be it is just unhandled.

@Lypsolon
Copy link

Actually before this issue was identified it never occurred to me. I think the correct response would be 400 as it is a problem with the request. Server should handle it as such and tell the client there's a problem with the request payload. 500 implies server error. which is not the case - or - it shouldn't be it is just unhandled.

If you want to change the response, that would be fine. (this means changes for the C++ API, but that's doable.) The only question for me is: Do we have the option to define all important responses before we make the change, aka not in this PR, and maybe we will end up with a file that we can reference? This would make it easy to reference, and maybe we can find more places like this. It's just a Friday thought.

@martastain
Copy link
Member Author

Sure. I'll merge this linked PR, close this issue and create a new issue when we could discuss it further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants