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

proposal: net: add MarshalText/UnmarshalText to HardwareAddr #29678

Open
maja42 opened this issue Jan 11, 2019 · 36 comments
Open

proposal: net: add MarshalText/UnmarshalText to HardwareAddr #29678

maja42 opened this issue Jan 11, 2019 · 36 comments
Labels
Milestone

Comments

@maja42
Copy link

maja42 commented Jan 11, 2019

What version of Go are you using (go version)?

$ go version
go1.10 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

I'm trying to unmarshal json content into net.IP and net.HardwareAddr.
Both types are actually of type []byte
Since net.IP implements json.Marshaler, it works as expected. However, net.HardwareAddr does not and fails with the error "illegal base64 data at input byte 2" (that's the position of the separator).

Here's an example:
https://play.golang.org/p/HOBBAyvpfrK

I found the google group discussion about adding the Marshaller to the net.IP type: https://groups.google.com/forum/#!topic/golang-nuts/io8aHJarm6U

What did you expect to see?

I expect net.HardwareAddr to be consistent with net.IP and implement json.Marshaler.
Strings in the form of "ab:cd:ef:ab:cd:ef" inside json should be parsable to MAC addresses.

@agnivade agnivade changed the title net, json: Add MarshalText/UnmarshalText to net.HardwareAddr proposal: net: Add MarshalText/UnmarshalText to HardwareAddr Jan 11, 2019
@gopherbot gopherbot added this to the Proposal milestone Jan 11, 2019
@agnivade agnivade changed the title proposal: net: Add MarshalText/UnmarshalText to HardwareAddr proposal: net: add MarshalText/UnmarshalText to HardwareAddr Jan 11, 2019
@agnivade
Copy link
Contributor

Considering there is a ParseMAC corresponding to ParseIP which is basically what the Unmarshaller for IP does, this seems like a reasonable request.

@rsc
Copy link
Contributor

rsc commented Jan 23, 2019

There are backwards compatibility concerns with adding MarshalText/UnmarshalText where they did not exist before. Was it possible to marshal these to JSON/Gob/XML before and then decode them back? If so, we can't break the old encoding. Or even if it was only possible to marshal, if that was a form other languages/tools knew, we can't change that either.

While we're on the topic, do we need to think about net.IPMask too?

@rsc rsc added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Feb 6, 2019
@rsc
Copy link
Contributor

rsc commented Feb 6, 2019

Waiting for someone to dig into whether there are compatibility issues here.

@agnivade
Copy link
Contributor

/cc @mikioh

@gopherbot
Copy link
Contributor

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@mikioh mikioh removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 7, 2019
@mikioh
Copy link
Contributor

mikioh commented Mar 7, 2019

Just FYI.

As far as I can see/remember;

  • textual representation for link- or below-layer identifiers varies,
  • the existing net.ParseMAC accepts only a few well-known representations, IEEE and some vendors, for the sake of convenience,
  • the existing net.HardwareAddr.String method returns the form described in IEEE Std 802-2001, Section 3.1.8 hexadecimal representation with the pick of "colons" instead of "hyphens",
  • though, MAC-48 is now obsolete. IEEE RA states that "... octets separated by hyphens; the IEEE RA refers to this as the hexadecimal (hex) representation." in https://standards.ieee.org/content/dam/ieee-standards/standards/web/documents/tutorials/eui.pdf

@mikioh mikioh reopened this Mar 7, 2019
@mikioh
Copy link
Contributor

mikioh commented Mar 7, 2019

While we're on the topic, do we need to think about net.IPMask too?

Also net.IPNet? I don't think so. As shown in #30264, there is a need for flexible IP address printer but simple, single-form text marshaler and unmarshaler might not fit for the actual needs, not only for classical telco sectors but tech sectors.

@ianlancetaylor
Copy link
Member

Still looking for comments on compatibility issues.

@andybons andybons added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 27, 2019
@klnusbaum
Copy link

@rsc I've dug through the git history of the go repo and I can't seem to find anything that would indicate any backwards compatibility issues. If there are specific things you'd like me to look for, I'm more than happy to, but at this point, I don't see any harm in adding this function.

klnusbaum added a commit to klnusbaum/go that referenced this issue Sep 21, 2019
The HardwareAddr type has the ability to be transformed to and from a
string. However, this capability is not exposed in a manner suitable
for use with various forms of marshalling and unmarshaling of text
(e.g. JSON or YAML). Let's add the proper functions so that
HardwareAddr implements the TextMarshaller and TextUnmarshaler
interfaces from the encoding package.

Fixes golang#29678
klnusbaum added a commit to klnusbaum/go that referenced this issue Sep 21, 2019
The HardwareAddr type has the ability to be transformed to and from a
string. However, this capability is not exposed in a manner suitable
for use with various forms of marshalling and unmarshaling of text
(e.g. JSON or YAML). Let's add the proper functions so that
HardwareAddr implements the TextMarshaller and TextUnmarshaler
interfaces from the encoding package.

Fixes golang#29678
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/196817 mentions this issue: net: add text marshalling and unmarshalling for HardwareAddr

@ianlancetaylor
Copy link
Member

@klnusbaum The backward compatibility issue is that someone could have marshaled net.HardwareAddr values to JSON using Go 1.13, and might try to unmarshal them using Go 1.14. The unmarshaling would fail because the format would be different.

Is there some reason that that is impossible?

@klnusbaum
Copy link

klnusbaum commented Sep 22, 2019

@ianlancetaylor I'm a little confused here. If someone marshaled a hardware address on their own into JSON, why would Go be on the hook for being able to unmarshal that value with this newly introduced code? That seems somewhat unreasonable to me.

@ianlancetaylor
Copy link
Member

In general, it seems reasonable that if you marshal a net.HardwareAddr into JSON with one Go release, you should be unmarshal that JSON into a net.HardwareAddr in a subsequent release. As far as I know that works today--you can marshal and unmarshal a net.HardwareAddr.

@klnusbaum
Copy link

As far as I know that works today--you can marshal and unmarshal a net.HardwareAddr.

Ah, ok. So this I think is the source of my confusion. I'm unaware of any way to marshal or unmarshal a net.HardwareAddr to JSON (for example) using the existing Go or any other previous version of Go. Can you point me to where this is possible?

@klnusbaum
Copy link

unless you're talking about a developer manually calling String and then writing that out to JSON. And then later, getting the string back and manually calling ParseMAC on it. In which case, the solution I've proposed in my PR would indeed be backwards compatible.

@zigazeljko
Copy link

Can you point me to where this is possible?

net.HardwareAddr has a underlying type of []byte and is therefore marshaled/unmarshaled as a base64 string (default behavior of encoding/json for byte slices).

Since that is an unusual format for MAC addresses, I guess it wasn't widely used, so there shouldn't be many compatibility issues.

In general, it seems reasonable that if you marshal a net.HardwareAddr into JSON with one Go release, you should be unmarshal that JSON into a net.HardwareAddr in a subsequent release.

If that's an issue, the UnmarshalText method can use base64.Decode as a fallback in case net.ParseMAC fails.

@klnusbaum
Copy link

Ah, so in other words, we're saying that we must remain backwards compatible with the raw marshaling of bytes? i.e. the behavior of this (which will marshal using the default marshal behavior of []byte) should continue to work?

	addr, _ := net.ParseMAC("aa:bb:cc:dd:ee:ff")

	marshaledBytes, _ := json.Marshal(addr)
	
	var unmarshaled net.HardwareAddr
	json.Unmarshal(marshaledBytes, &unmarshaled)
	
	fmt.Printf("Unmarshalled Addr: %s", unmarshaled)

If that's the case, then yea, I think we can do as @zigazeljko suggested, fallback to raw byte unmarshaling in the case of ParseMAC failing. @ianlancetaylor do you think that would work?

@smasher164 smasher164 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 5, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 5, 2019
@smasher164 smasher164 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 5, 2019
@rsc
Copy link
Contributor

rsc commented Nov 6, 2019

The main thing the PR is missing is consensus on this proposal that we want to do this.
That in turn is blocked on understanding the compatibility issues.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2019

On hold for understanding the compatibility issues. I will try to do that at some point if no one else beats me to it.

@TheLillo
Copy link

TheLillo commented Jan 8, 2025

This issue is not updated as of August 10, 2022; any news?

@klnusbaum
Copy link

I took a stab a making a fix, but got hung up on maintaining backwards compatibility. Right now, if you write a MAC address out to, for example, yaml, its hex representation is output. So in order to not break things, we need to be able to:

  1. Determine what format the MAC Address is in (either the "legacy" hex or the the desired AA:BB:CC:DD:EE:FF format).
  2. Parse both formats

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/641935 mentions this issue: net: add readable JSON encoding of HardwareAddr

@ianlancetaylor
Copy link
Member

I wrote https://go.dev/cl/641935 which implements the expected JSON marshaling, and supports both the new JSON marshaling and the old one. This addresses the compatibility concern, in that we can unmarshal a marshaled HardwareAddr whether it came from an old version of Go or a future version of Go.

Taking off hold.

@ianlancetaylor ianlancetaylor removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Proposal-Hold labels Jan 9, 2025
@ianlancetaylor ianlancetaylor moved this from Hold to Incoming in Proposals Jan 9, 2025
@klnusbaum
Copy link

@ianlancetaylor this is great. Looks like you went with implementing the MarshalText interface. That means this should work with YAML as well as JSON, yea?

@ianlancetaylor
Copy link
Member

@klnusbaum Yes. For simple cases like this I can't see any reason to use MarshalJSON, MarshalText seems entirely sufficient.

@seankhliao
Copy link
Member

I believe based on the accepted proposal in #10275 (comment) , this wouldn't be allowed as it breaks marshaling on a new version and unmarshaling on an old version.

@ianlancetaylor
Copy link
Member

@seankhliao Thanks, I forgot about that. We'll have to decide whether this seems like an important enough case to worry about marshaling with new programs and unmarshaling with old ones. We could, of course, add a GODEBUG variable to get the old marshaling.

@maja42
Copy link
Author

maja42 commented Jan 12, 2025

Or it could be done for json/v2 only.

@seankhliao
Copy link
Member

Changing the output of serialization from using encoding/json to encoding/json/v2 would be a similar kind of break discussed in #10275, where if you care about compatibility between old and new versions of your code through serialized data, you can't do it.

@rsc rsc moved this from Incoming to Active in Proposals Feb 5, 2025
@rsc
Copy link
Contributor

rsc commented Feb 5, 2025

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Active
Development

Successfully merging a pull request may close this issue.