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.

Sign up for GitHub

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

Jump to bottom

encoding/xml: use typed error for "non-pointer passed to Unmarshal" #57557

Open
mojixcoder opened this issue Jan 3, 2023 · 12 comments
Open

encoding/xml: use typed error for "non-pointer passed to Unmarshal" #57557

mojixcoder opened this issue Jan 3, 2023 · 12 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@mojixcoder
Copy link

mojixcoder commented Jan 3, 2023 •
edited

In the encoding/xml package when a non-pointer is passed to the function, it returns a new error with errors.New function.
The problem is that we cannot handle errors like this:

err := xml.NewDecoder(readCloser).Decode(out)
// How to explicitly handle a non-pointer passed to Unmarshal in here?
if err == ??? {
}

encoding/json returns *json.InvalidUnmarshalError when a non-pointer is passed and it can be handled on the client side because it doesn't return the error with errors.New function.

It happens in here:

go/src/encoding/xml/read.go

Line 150 in db36eca

return errors.New("non-pointer passed to Unmarshal")

@beoran
Copy link

beoran commented Jan 3, 2023

How about this?

if err.Error() == "non-pointer passed to Unmarshal" {
// Handle error here.
}

@mojixcoder
Copy link
Author

mojixcoder commented Jan 3, 2023 •
edited

How about this?

if err.Error() == "non-pointer passed to Unmarshal" { // Handle error here. }

It also works but it's not considered the best practice to handle errors by using their string value.
That's one of the reasons that so many packages have pkg.ErrXXX errors for better and easier error handling.
Also for handling errors this way you either need to print the error value or reading the source code and copying the error string that is used in the function to be able to compare it with another error string.

@seankhliao seankhliao changed the title How to handle non-pointer error returned by XML decoder's method encoding/xml: use typed error for "non-pointer passed to Unmarshal" Jan 3, 2023
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 3, 2023
@ianlancetaylor
Copy link
Contributor

Generally it's useful to have a typed error for an error that can be caused by user input, where the program may need to take different action depending on what it sees. It's less clear that that's useful for a case like this where the only possible fix is to change the program. When would it be useful for a program to explicitly detect this case? What would it do when it happens?

@ianlancetaylor
Copy link
Contributor

I would suggest the 500 response for any unrecognized error. Does it help to categorize this particular error?

@mojixcoder
Copy link
Author

I would suggest the 500 response for any unrecognized error. Does it help to categorize this particular error?

How many recongnized errors do we have?
At least 3 or 4 in this case, I think it's better to handle a specific error instead of writing 3 or 4 extra checks for an unrecognized error.
I think it will be even better to validate user input manually instead of writing extra checks because of an unrecognized error.

@mojixcoder
Copy link
Author

mojixcoder commented Jan 3, 2023 •
edited

Generally it's useful to have a typed error for an error that can be caused by user input, where the program may need to take different action depending on what it sees. It's less clear that that's useful for a case like this where the only possible fix is to change the program. When would it be useful for a program to explicitly detect this case? What would it do when it happens?

Sorry I accidentally deleted my reply to this comment, so I am writing it again.
For example when someone is writing a web framework and wants to read request body as JSON.
When the programmer passes a non-pointer value the framework should panic or send a 500 response because the error comes from the developer.
But when a client calls our service and sends an invalid XML payload it's better not to panic or send a 400 error instead of 500.

@seankhliao
Copy link
Member

Indeed, and there's a xml.SyntaxError for that case.
There will be a wide variety of errors (eg from the underlying reader) that can be returned, I think the expectation is to special case the few known ones related to user input and report those appropriately, with a default for everything else. After all, Go functions don't (and most can't) give an exhaustive list of errors they can return.

@mojixcoder
Copy link
Author

mojixcoder commented Jan 3, 2023 •
edited

Indeed, and there's a xml.SyntaxError for that case. There will be a wide variety of errors (eg from the underlying reader) that can be returned, I think the expectation is to special case the few known ones related to user input and report those appropriately, with a default for everything else. After all, Go functions don't (and most can't) give an exhaustive list of errors they can return.

I know there is xml.SyntaxError.
Actually now I'm handling it like err.Error() == "my string".
I just faced a situation in which I thought it would be better if I could handle it like this as usually most people do that and it's considered a best practice.
It's definitly not critical or something that can't be handled in other ways at all, I just wanted to introduce a better way.

@jacobishao
Copy link
Contributor

jacobishao commented Jan 7, 2023 •
edited

@mojixcoder, I am a bit curious about why you need handle this error. could you please give some explanation?In my option, say that if you passed a non-pointer variable to the function, when you fix it, the error will never appear again.

@mojixcoder
Copy link
Author

@mojixcoder, I am a bit curious about why you need handle this error. could you please give some explanation?In my option, say that if you passed a non-pointer variable to the funciton, when you fix it, the error will never appear again.

I've already said that:
#57557 (comment)

@jacobishao
Copy link
Contributor

@mojixcoder Sorry. how about just returning the error to web framework user? just like Gin framework
https://github.com/gin-gonic/examples/blob/master/basic/main.go#L61

@mojixcoder
Copy link
Author

@jacobishao I'm returing error at the end but wanted to be able to return different errors.
Actually I'm handling it with err.Error() now.

@seankhliao seankhliao added this to the Unplanned milestone Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@beoran @ianlancetaylor @jacobishao @seankhliao @mojixcoder