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

encoding/xml: add a tag for setting a default value during unmarshalling when an attribute is not present #21425

Closed
onitake opened this issue Aug 13, 2017 · 9 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@onitake
Copy link

onitake commented Aug 13, 2017

This is a proposal for a new feature in the encoding/xml package.

I'm currently working with the aqwari.net/xml package to generate XML parsers and validators for XSD schemas. The generated parsers and generators use encoding/xml under the hood. For many use cases, this is sufficient, but it does not cover some more advanced features without re-implementing functionality that is already present in encoding/xml.

In particular, I would like to set default values for attributes, when they are specified in the XSD. This could be implemented in an encoding.TextUnmarshaler.UnmarshalText method for the specific attribute, but since UnmarshalText does not receive any context information, it's not possible to know what the default value should be. The same applies to xml.UnmarshalerAttr.UnmarshalXMLAttr. It may be possible to do it with a custom xml.Unmarshaler.UnmarshalXML, but this significantly more work and duplicates functionality of encoding/xml.

So, I propose the following change: Add support for a new xml:default=string tag to encoding/xml, possibly somewhere in /src/encoding/xml/typeinfo.go, that will set an attribute to string if the data to be decoded does not contain this attribute. This could be done before any data is unmarshalled, or during decoding.

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 14, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Aug 14, 2017
@AlexRouSg
Copy link
Contributor

This could be useful for JSON too, which I use.

@rsc
Copy link
Contributor

rsc commented Aug 21, 2017

If you want to know whether a value was set, unmarshal into *Value instead of Value. It will be nil if not present, allocated otherwise. Having a 'default=' means parsing defaults, which is more complexity than we really want. And it will interfere with every kind of value we unmarshal into, multiplying the complexity. This doesn't seem common enough to be worth the extra complexity when there's a decent solution already.

@onitake
Copy link
Author

onitake commented Aug 22, 2017

Using a pointer instead of an embedded value is a useful hint, thank you.

But it is not a decent solution, because it still requires custom default value handling after deserialisation. Worse yet, all processed data structures need to be post-processed so all the nil values are replaced with valid values - or code that uses them needs to be adapted to cope with empty pointers. Depending on the use case, that may not be desirable.

I agree that adding such a feature will increase complexity of the XML module, but it's most likely less than the alternatives. That said, I'm discussing other approaches to tackling the issue with the developer of aqwari.net/xml. If we find a useful (and non-obvious) solution, it may be worth adding it to the xml module documentation?

@plandem
Copy link

plandem commented Aug 22, 2017

pointer is ok for structs, but for primitive types it's critical to have default value. E.g.: we want optional unsigned int. Right now it's not possible. With default value we could use -1 and custom type to correctly marshal/unmarshal it:

type UnsignedInt int
func (e *UnsignedInt) MarshalXMLAttr(name xml.Name) (xml.Attr, error) {
	if *e == -1 {
		return xml.Attr{}, nil
	}

	return xml.Attr{Name: name, Value: strconv.Itoa(int(*e))}, nil
}

func (e *UnsignedInt) UnmarshalXMLAttr(attr xml.Attr) error {
	*e = -1

	if len(attr.Value) > 0 {
		if index, err := strconv.ParseInt(attr.Value, 10, 32); err == nil {
			*e = UnsignedInt(index)
		}
	}

	return nil
}

and use it like this:

type StyleRef struct {
	NumFmtId          UnsignedInt `xml:"numFmtId,attr, default:-1"`
	FontId            UnsignedInt `xml:"fontId,attr, default:-1"`
	FillId            UnsignedInt `xml:"fillId,attr, default:-1"`
}

Your advice to use *uint will work, but has 2 drawbacks:

  1. double usage of memory (pointer to uint, instead of uint) - maybe critical in some cases
  2. from codebase, it's pain in arse to unreference it everywhere

P.s.: I would suggest default value only for primitive types

@onitake
Copy link
Author

onitake commented Aug 22, 2017

Default values are actually only relevant for attributes, and attributes can only have simple or built-in types, according to the XSD spec.

@rsc
Copy link
Contributor

rsc commented Oct 30, 2017

My comment above (#21425 (comment)) explains a workaround. The specific motivation here is not common enough to warrant new complexity in encoding/xml.

@rsc rsc closed this as completed Oct 30, 2017
@onitake
Copy link
Author

onitake commented Nov 14, 2017

Thanks for clearing that up, even though the workaround is not an ideal solution and requires more boilerplate code than necessary. But there's probably a solution that isn't too awkward to use.
We'll keep trying.

@carldunham
Copy link

the workaround is not an ideal solution and requires more boilerplate code than necessary.

Go in a nutshell

@Noofbiz
Copy link

Noofbiz commented Aug 29, 2018

I ended up here looking for the answer to this problem. I actually found another solution on Stack Overflow here and just wanted to leave it here for anyone looking. It doesn't solve the multiple copies in memory, but I thought it was a nicer solution, particularly for built-in types.

@golang golang locked and limited conversation to collaborators Aug 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

8 participants