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

Add response string to repodata JSONDecodeError #13128

Merged
merged 5 commits into from
Feb 28, 2024

Conversation

dholth
Copy link
Contributor

@dholth dholth commented Sep 19, 2023

Description

Fix #13110

This should make it much easier to figure out what is going on when repodata.json is not parseable.

Do we need to sanitize the response so it can't print HTML or escape characters?

It would be better if we converted this to a CondaError instead of surfacing the Python exception. We could raise a subclass of JSONDecodeError with the data and convert to CondaError at a much higher level?

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@dholth dholth requested a review from a team as a code owner September 19, 2023 22:04
@dholth dholth self-assigned this Sep 19, 2023
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Sep 19, 2023
@travishathaway
Copy link
Contributor

travishathaway commented Sep 20, 2023

@dholth,

It think it would be a better idea to address these issues just slightly further up the call stack.

What if instead of returning a JSON string that has be parsed later, fetch_latest always returned a parsed value and we forced it to be dealt with there?

We could also change read_cache to work the same way too. That way we could also separate the error messages so that it's easier for us differentiate invalid JSON coming from a network request vs. coming from bad cache values on disk.

Also, I would expect the error message to look similar to what we have in read_cache right now for problems related to faulty copies of cached data.

In the future, it might also be nice to figure out how to implement some retry logic if a faulty cached version is found (i.e. just remove it and grab another fresh copy from the server).

@dholth
Copy link
Contributor Author

dholth commented Sep 20, 2023

@travishathaway this was made so that we can have have fetch_latest_path and pass the repodata file to something like libmamba without necessarily parsing.

The user that is having this issue gets the same issue after clearing the cache, their server or firewall may be returning not-json.

f'{e.args[0]}; got "{parsed[:ERROR_SNIPPET_LENGTH]}"',
*e.args[1:],
)
raise
Copy link
Contributor Author

@dholth dholth Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way for me to get the URL here... (without any tokens?)

@jezdez jezdez changed the title add response string to repodata JSONDecodeError Add response string to repodata JSONDecodeError Sep 25, 2023
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we create a higher level tool that wraps json.loads or something that would enable us to handle this holistically through out the codebase?

@dholth dholth added the build::review trigger a build for this PR label Sep 25, 2023
@conda-bot conda-bot removed the build::review trigger a build for this PR label Sep 25, 2023
@conda-bot
Copy link
Contributor

conda-bot commented Sep 25, 2023

Review build status

The workflow for building and uploading a canary release for conda with the label conda-conda-pr-13128 in channel conda-canary was started by @dholth!

Once it's done, use this command to try it out in a new conda environment:

conda install -c conda-canary/label/conda-conda-pr-13128 conda

@dholth
Copy link
Contributor Author

dholth commented Sep 29, 2023

I'm not sure about a global wrapper for json.loads(). Could be tricky to decide when to keep the input string or figure out where to attach additional useful information such as the URL that produced the unparseable input.

A good followup would be to write a test that adds a bad channel (html-formatted repodata.json) and check that conda still loads any other configured channels.

@dholth
Copy link
Contributor Author

dholth commented Oct 3, 2023

The customer was able to run this branch and see the beginnings of a HTML document in the traceback. Then we were able to change a URL in condarc.

Copy link

codspeed-hq bot commented Jan 25, 2024

CodSpeed Performance Report

Merging #13128 will create unknown performance changes

Comparing 13110-improved-jsondecodeerror (a6f2629) with main (9c2a156)

Summary

⚠️ No benchmarks were detected in both the base of the PR and the PR.

@kenodegard
Copy link
Contributor

pre-commit.ci autofix

@kenodegard kenodegard enabled auto-merge (squash) February 28, 2024 16:21
@kenodegard kenodegard merged commit 0bf33e9 into main Feb 28, 2024
78 checks passed
@kenodegard kenodegard deleted the 13110-improved-jsondecodeerror branch February 28, 2024 18:29
@jezdez jezdez mentioned this pull request Mar 11, 2024
58 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Friendly JSONDecodeError alternative when repodata.json can't be parsed?
6 participants