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

How we check if a schema is local or online #138

Open
ghost opened this issue Nov 14, 2023 · 2 comments
Open

How we check if a schema is local or online #138

ghost opened this issue Nov 14, 2023 · 2 comments

Comments

@ghost
Copy link

ghost commented Nov 14, 2023

From #137

The change to if schema_url.startswith("http") needs to be carried over. It's necessary to "fix(custom_jsonref_jsonloader): Read from filesystem if schema_url is a file path", because lib-cove has already messed around with the uri value by this point (IIRC), and checking schema_url directly is the only way to know if the URL is a file path or not.

Edit: Indeed - uri is parsed, and then uri is set to uri = urljoin(schema_url, ...). So, if schema_url is a file path, then "http" in uri_info.scheme will be True, even though the subsequent GET request will use a value of uri that's prefixed with a file path (i.e. no scheme). So, it's necessary to check schema_url instead of uri. This is more correct, even if this particular test doesn't need it.

@ghost
Copy link
Author

ghost commented Nov 24, 2023

@jpmckinney How important do you think it is to do this one? And is it for the purposes of making tests easier or does it affect normal running? I know the number of lines of code changed is tiny, but I would want to fully understand the code path and other things that use this so it may take a while to do.

For reference, change to libcove/lib/common.py, custom_jsonref_jsonloader func, inner jsonloader func:

if "http" in uri_info.scheme:

To

 schema_url.startswith("http")

@jpmckinney
Copy link
Contributor

Hmm, thanks to #136 I circumvent this issue entirely, so I don't need this fixed.

That said, I would keep the issue open, as it is a bug, and for my part, I've confirmed that the fix doesn't negatively affect any other code paths, etc.

The code paths are quite gnarly as this code is very much action-at-a-distance, so I understand that it would take time to re-validate.

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

No branches or pull requests

1 participant