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

Fix key-error when response returns no json #219

Merged
merged 7 commits into from
Oct 30, 2020
Merged

Fix key-error when response returns no json #219

merged 7 commits into from
Oct 30, 2020

Conversation

EddyK69
Copy link
Contributor

@EddyK69 EddyK69 commented Oct 30, 2020

Breaking change

Proposed change

When certain service if not available (RangeMap for example), the response.json() does not contain the service-key.
This PR fixes the unhandled exception and prevents stopping service-loop.

Type of change

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to this library)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Also extended the data_anonymizer :D

Checklist

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works.

@rikroe
Copy link
Member

rikroe commented Oct 30, 2020

Many thanks for this, will definetly include the extended anonymizer!
Regarding failed services, I'd like your opinion on #213 (I am trying to fix the source of the issue there).

@EddyK69
Copy link
Contributor Author

EddyK69 commented Oct 30, 2020

Regarding failed services, I'd like your opinion on #213 (I am trying to fix the source of the issue there).

I will look at this, but, my car supports RangeMaps but the request returns 500 most of the times, sometimes 200. So, my fix wil handle those request returning no valid key in the request.json(). This has nothing to do with the car support a service or not...

@rikroe
Copy link
Member

rikroe commented Oct 30, 2020

Ah, that is good to know! Then it is for sure better to also account for this!

@EddyK69
Copy link
Contributor Author

EddyK69 commented Oct 30, 2020

Regarding failed services, I'd like your opinion on #213 (I am trying to fix the source of the issue there).

@rikroe PR #213 seems good to me! No issues found...
But my rangeMap attribute is "RANGE_POLYGON" instead of "RANGE_CIRCLE", so it returns that is not supported (see my fingerprint-file in #191

@rikroe
Copy link
Member

rikroe commented Oct 30, 2020

Thanks! Regarding RANGE_POLYGON/RANGE_CIRCLE: Do you sometimes get a valid response?
If not it might make sense to adjust the code of #213 as RANGE_POLGYON seems to have different requirements (app security? additional passwords?) than RANGE_CIRCLE so we can better control when to use which data.

@rikroe rikroe merged commit 35c98df into bimmerconnected:master Oct 30, 2020
@EddyK69
Copy link
Contributor Author

EddyK69 commented Nov 1, 2020

I tried to get a valid response the last few days, but i only get 500’s, not a single 200 unfortunately. I agree on adding the #213 to resolve this, although my Rangemap != UNSUPPORTED

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants