Replace wrong domain returned from xbox api 2.0 #47021
Conversation
probot-home-assistant
bot
added
bugfix
integration: xbox
small-pr
|
I don't understand what's going wrong here. The error logs of the check that failed references a file i haven't touched, but somehow it fails? How can i fix this? |
| url = URL(self.data.display_pic) | ||
| if url.host == "images-eds.xboxlive.com": | ||
| url = url.with_host("images-eds-ssl.xboxlive.com") | ||
| return str(url.with_query(url=url.query["url"], format=url.query["format"])) |
There was a problem hiding this comment.
None of this is needed i think. You just need str(url). Everything else should have been retained.
There was a problem hiding this comment.
Also you seem to have lost the padding removal.
There was a problem hiding this comment.
I don't understand, you commented that I needed to check if the url needs replacing which I added.
This is the most elegant solution I could think of, because now it checks if the base url needs replacement and does that when necessary (row 56 and 57).
The on row 58, it returns the url with the query string which contains only the necessary queries url and format and thus removes the padding.
How would you do this differently? You commented something like this before, I think I misunderstood then.
There was a problem hiding this comment.
The old code removed a padding url argument. The new doesn't.
There was a problem hiding this comment.
Okey you recreate with known url queries. But that is not equivalent.
It should have done something like.
q = url.query
q.pop("padding, None)
url = with_query(q)
To be equivalent.
There was a problem hiding this comment.
Okay, i'll try that
Checkes have passed, do i need to do anything else to be able to get it merged or is it considered done? |
Somehow after setting up the dev environment, i couldn't commit/push to my original fork, thus i needed to create a seperate branch and push it into that. As far as i know i can't merge that branch with the original pull request #46723, if that's somehow possible, please let me know
Proposed change
As explained in this issue-807727457 the xbox api somehow returns a wrong domain sometimes. The wrong url is: "https://images-eds.xboxlive.com/" and it contains a wrong certificate (common name invalid). However, if you replace the domain with: "https://images-eds-ssl.xboxlive.com/" there is no certificate error and the correct image is loaded.
To fix this, the base_sensor can do a string replace. if the wrong url is returned, it is replaced with the correct one. If the xbox api returns the correct one already, nothing is replaced.
**I don't have the means or knowhow to test this, as its a simple change, i'm hoping someone can test this or help me test this **
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
[N/A]
If the code communicates with devices, web services, or third-party tools:
[N/A]
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: