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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix faultcode+faultstring and get UPnPError details #60

Merged
merged 3 commits into from
May 10, 2023

Conversation

jybp
Copy link
Contributor

@jybp jybp commented May 10, 2023

馃憢 A small PR that does two things:

  1. Fixes the parsing of faultcode/faultstring fields.
    The change was introduced in this PR but I'm not sure why? Improve error handling聽#38
    You've also properly set them in v2alpha: https://github.com/huin/goupnp/blob/main/v2alpha/soap/envelope/envelope.go#L23:L24
    And the specs clearly specify this case: https://openconnectivity.org/upnp-specs/UPnP-arch-DeviceArchitecture-v2.0-20200417.pdf
    See page 74 as well as examples pages 83 & 84.

  2. Parses some details of the optional UPnPError element. This will allow consumers that want to inspect the error to be able to easily retrieve errorCode and errorDescription. The Raw field is still there as a catch-all.

@jybp jybp marked this pull request as ready for review May 10, 2023 11:41
@jybp
Copy link
Contributor Author

jybp commented May 10, 2023

cc @huin

@huin
Copy link
Owner

huin commented May 10, 2023

Thanks, looks good.

There's a slight risk that existing code is pulling UPnPError out of Raw. I wonder if we should instead provide parsing methods on the fault type to extract this as a named struct, so that existing code won't change behaviour as a result. Otherwise this change would be good for a major version.

@jybp
Copy link
Contributor Author

jybp commented May 10, 2023

What do you mean by risk of code pulling out from Raw?
Raw should always contain the whole content of detail, no matter if Errorcode/ErrorDescription have been set (like in the test). The change shouldn't be disruptive to existing codebases that don't know about the new UPnPError exported field, but rely on Raw.

If you think just adding fields to SOAPFaultError still requires a major version, we can provide a helper like you suggested: c460521
Otherwise, the last commit can be reverted.

@huin
Copy link
Owner

huin commented May 10, 2023

What do you mean by risk of code pulling out from Raw?

Yes.

But I hadn't appreciated that that's how the XML unpacking worked - thanks for explaining that. In that case we can go with your original changes.

@jybp
Copy link
Contributor Author

jybp commented May 10, 2023

Cool, I removed that last change then. LMK when that can be merged & tagged 馃憤

@huin huin self-requested a review May 10, 2023 16:06
@huin huin merged commit 8ca2329 into huin:main May 10, 2023
@huin
Copy link
Owner

huin commented May 10, 2023

That's merged in. Thanks :)

@jybp
Copy link
Contributor Author

jybp commented May 12, 2023

Any chance a v1.2.0 could be introduced with this change ? :)

@huin
Copy link
Owner

huin commented May 12, 2023

Heh, woops. I actually drafted the release, and then failed to publish it. Done. :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants