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
encoding/xml: Unmarshalling empty XML attribute to int throws error #19333
Comments
Recently tried upgrading to 1.8 and ran into this. Many of our XML-based 3rd parties leave attributes empty for prices, |
This appears to have been introduced by 2c58cb3 which returns an error that was previously ignored. This is a wider change that may have affected several types; I'm unsure if it breaks the compatibility promise, or was fixing a bug. I'm inclined to think that it doesn't make sense for ints to unmarshal to their zero value in XML, but this is one of those weird cases where Go types/XML aren't really compatible and any mapping between them will have to do some funky stuff, and if it's always been doing this maybe we should special case it and ignore the err result of strconv. /cc @ericlagergren (author), @bradfitz (reviewer) |
@rsc, do you have opinions here? |
Also worth noting: before that commit unmarshaling values did return an strconv error, it was only unmarshaling attributes that ignored it. Test:
|
Like @SamWhited said, it makes the most sense to special case I created the original issue because I tried to marshal a single letter into a The alternative (reverting 2c58cb3) means if the client sends an invalid XML request (e.g., I believe that behavior is definitely a bug in the XML package. |
related to #13417 |
Fixed via 0a0186f |
Reopening for backport I guess. |
I can't figure out what is going on here. The linked commit 0a0186f changes the behavior of the XML package beyond this bug fix. It is not appropriate for Go 1.8.1. Leaving this open because we need to decide whether to live with the bug or prepare a different release-specific fix. |
Could we revert 2c58cb3 for 1.8.1 only? |
Russ—I think the linked commit only happened to fix this issue and was in
response to another bug. See Sarah Adams' comments.
…On Wed, Apr 5, 2017 at 7:22 AM Sam Whited ***@***.***> wrote:
Could we revert 2c58cb3
<2c58cb3>
for 1.8.1 only?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19333 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AFnwZ9PBKXM9qfwHhLsA_k1DPtfG6JVpks5rs6OKgaJpZM4MPDYh>
.
|
OK but the bug was marked Go 1.8.1 and closed with a link to that commit. That commit is an inappropriate fix for Go 1.8.1 (and not cherry-picked yet anyway), so reopening. |
@SamWhited, yes, that seems to be the solution. I will send a CL. |
My apologies for closing. |
No worries @adams-sarah, it's all very confusing around point release milestones. We hope to have a bot help with this soon. |
Here is a test program. It covers the cases in this issue (attributes) and the ones in #13417 (elements):
Go 1.2 through Go 1.7 were consistent: attributes unchecked, children strictly checked:
Go 1.8 made attributes strictly checked, matching children:
Go 1.9 will (at least right now) relax things so that only non-empty bad inputs are checked:
For Go 1.8.1, I will revert the checking behavior back to Go 1.7 for now. We can try another attempt at attribute checking, with @adams-sarah's strictness relaxation applied both to attributes and children, in Go 1.9.
|
CL https://golang.org/cl/39607 mentions this issue. |
Cherry-picked. |
…ntax, like Go 1.7 Consider this struct, which expects an attribute A and a child C both ints: type X struct { XMLName xml.Name `xml:"X"` A int `xml:",attr"` C int } Go 1.2 through Go 1.7 were consistent: attributes unchecked, children strictly checked: $ go1.7 run /tmp/x.go <X></X> ok <X A=""></X> ok <X A="bad"></X> ok <X></X> ok <X><C></C></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X><C/></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X><C>bad</C></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax $ Go 1.8 made attributes strictly checked, matching children: $ go1.8 run /tmp/x.go <X></X> ok <X A=""></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X A="bad"></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax <X></X> ok <X><C></C></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X><C/></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X><C>bad</C></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax $ but this broke XML code that had empty attributes (#19333). In Go 1.9 we plan to start allowing empty children (#13417). The fix for that will also make empty attributes work again: $ go run /tmp/x.go # Go 1.9 development <X></X> ok <X A=""></X> ok <X A="bad"></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax <X></X> ok <X><C></C></X> ok <X><C/></X> ok <X><C>bad</C></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax $ For Go 1.8.1, we want to restore the empty attribute behavior to match Go 1.7 but not yet change the child behavior as planned for Go 1.9, since that change hasn't been through release testing. Instead, restore the more lax Go 1.7 behavior, so that XML files with empty attributes will not be broken until Go 1.9: $ go run /tmp/x.go # after this CL <X></X> ok <X A=""></X> ok <X A="bad"></X> ok <X></X> ok <X><C></C></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X><C/></X> ERROR strconv.ParseInt: parsing "": invalid syntax <X><C>bad</C></X> ERROR strconv.ParseInt: parsing "bad": invalid syntax $ Fixes #19333. Change-Id: I3d38ebd2509f5b6ea3fd4856327f887f9a1a8085 Reviewed-on: https://go-review.googlesource.com/39607 Run-TryBot: Russ Cox <rsc@golang.org> Reviewed-by: Sarah Adams <shadams@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
Go version:
1.8 (regression from 1.7.x --> 1.8.0)
Environment:
Dockerized Alpine (golang:1.8-alpine and golang:1.7-alpine)
Description:
In Go 1.7, an XML entity like
<Object foo=""></Object>
could be unmarshalled into a struct whereFoo
is anint
value without throwing an error. When upgrading to Go 1.8.0, the same code throws an error due tostrconv.ParseInt
being called with the empty string as an argument.This seems to be a behavioral regression between Go 1.7.x and Go 1.8.0.
Example code:
https://play.golang.org/p/GIsONzXQQQ
Expected behavior:
Example code should return without error, printing
Foo: 0
Actual behavior:
An error is thrown, indicating that ParseInt was called with an empty string.
Compiling this same code with Go 1.7.x results in the expected behavior.
The text was updated successfully, but these errors were encountered: