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

fix: xml marshal byte array fields as vCenter does #3476

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

dougm
Copy link
Member

@dougm dougm commented Jun 24, 2024

Go's encoding/xml package and vCenter marshal '[]byte' differently:
Go encodes the entire array in a single xml element, for example:

  <foo>
    <bar>hello</bar>
  </foo>

vCenter encodes each byte of the array in its own xml element, example with same data as above:

  <foo>
    <bar>104</bar>
    <bar>101</bar>
    <bar>108</bar>
    <bar>108</bar>
    <bar>111</bar>
  </foo>

This behavior is hardwired, see the xml/encoding source for the handful of []byte special cases along the lines of:

  if reflect.Slice && slice element type == reflect.Uint8

While govmomi maintains a fork of Go's xml/encoding package, we prefer to further diverge.
Proposed solution is to use a 'ByteSlice' type that implements xml marshaling as vCenter does,
but otherwise behaves just as []byte does.

Commits that follow enhance vcsim and govc support around various []byte fields.

Fixes #1977
Fixes #3469

simulator/host_certificate_manager.go Fixed Show fixed Hide fixed
vim25/types/helpers.go Fixed Show fixed Hide fixed
@dougm dougm force-pushed the fix-byte-array branch 3 times, most recently from 336e7f3 to 0a591b7 Compare June 24, 2024 02:36
@dougm dougm changed the title fix: properly xml marshal byte array fields fix: xml marshal byte array fields as vCenter does Jun 24, 2024
Copy link
Member

@akutz akutz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dougm,

The ByteArray change looks good with a few nits. It did take me a moment to realize why you defined ByteArray instead of extending ArrayOfBytes. I wonder if it makes sense to instead name type ByteArray as type ByteSlice to further distinguish the two types. Because ByteArray really is a []byte (slice of bytes), so I think I prefer type ByteSlice. That way we can state the WSDL type ArrayOfBytes internally uses a ByteSlice in GoVmomi.

Thoughts?

vim25/types/helpers.go Outdated Show resolved Hide resolved
vim25/types/helpers.go Outdated Show resolved Hide resolved
Go's encoding/xml package and vCenter marshal '[]byte' differently:
- Go encodes the entire array in a single xml element, for example:
  <foo>
    <bar>hello</bar>
  </foo>

- vCenter encodes each byte of the array in its own xml element, example with same data as above:
  <foo>
    <bar>104</bar>
    <bar>101</bar>
    <bar>108</bar>
    <bar>108</bar>
    <bar>111</bar>
  </foo>

This behavior is hardwired, see the xml/encoding source for the handful of []byte special cases along the lines of:

  if reflect.Slice && slice element type == reflect.Uint8

While govmomi maintains a fork of Go's xml/encoding package, we prefer to further diverge.
Proposed solution is to use a 'ByteSlice' type that implements xml marshaling as vCenter does,
but otherwise behaves just as '[]byte' does.

Commits that follow enhance vcsim and govc support around various []byte fields.

Fixes vmware#1977
Fixes vmware#3469
Copy link
Member

@akutz akutz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, thanks Doug!

@dougm dougm merged commit 34c3788 into vmware:main Jun 24, 2024
11 checks passed
@dougm dougm deleted the fix-byte-array branch June 24, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants