XML tags fixed for correct unmarshalling of ResourcePoolRefs in AdminVdc and test added

[ArnoSen]

IMPORTANT

To help us process your pull request efficiently, please follow the guidelines shown below.

A Pull Request should be associated with an Issue

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs. If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues, and potentially we'll be able to point development in a particular direction.

We accept PRs without associated issues provided the change is sufficiently evident from the commit message. If you have typos or simple bug fixes go for it.

Description

Related issue:

• (Required) Short description of changes in the PR subject

• (Required) Detailed description of changes include tests and documentation. If the pull request contains multiple commits with detailed messages, refer to those instead

XML tags have been corrected and a test has been added that shows the xml is properly unmarshalled.

• (Optional) Names of reviewers using @ sign + name

[vmwclabot]

@ArnoSen, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

[lelvisl]

Waiting for VimObjectRef fixed in this PR to fix same situation with AdminVM

[vbauzys]

Hi @ArnoSen

thank you for your improvement.

Two things to make it better. It's enough to improve test Test_CreateNsxtOrgVdc by adding asserts to check that ResourcePoolRefs are returned.

And please add an improvement note in .changes/v2.16.0 folder as a file.

Thank you.

[ArnoSen]

Hi @ArnoSen

thank you for your improvement.

Two things to make it better. It's enough to improve test Test_CreateNsxtOrgVdc by adding asserts to check that ResourcePoolRefs are returned.

And please add an improvement note in .changes/v2.16.0 folder as a file.

Thank you.

The two things you asked for have been added.

[ArnoSen]

Waiting for VimObjectRef fixed in this PR to fix same situation with AdminVM

I am unable to find any object named AdminVM . There is AdminVMRecord but that is not referencing VimObjectRef.

[vmwclabot]

@ArnoSen, VMware has approved your signed contributor license agreement.

[vbauzys]

START: adminvdc_nsxt_test.go:19: TestVCD.Test_CreateNsxtOrgVdc
adminvdc_nsxt_test.go:129:
    check.Assert(err, IsNil)
... value *errors.errorString = &errors.errorString{s:"error updating VDC: API Error: 400: [ 543249b7-69b2-4dcf-aa36-9d4caf6f2b62 ] HTTP 400 Bad Request\n - cvc-complex-type.2.4.a: Invalid content was found starting with element '{\"http://www.vmware.com/vcloud/v1.5\":VimObjectRef}'. One of '{\"http://www.vmware.com/vcloud/extension/v1.5\":VimObjectRef}' is expected."} ("error updating VDC: API Error: 400: [ 543249b7-69b2-4dcf-aa36-9d4caf6f2b62 ] HTTP 400 Bad Request\n - cvc-complex-type.2.4.a: Invalid content was found starting with element '{\"http://www.vmware.com/vcloud/v1.5\":VimObjectRef}'. One of '{\"http://www.vmware.com/vcloud/extension/v1.5\":VimObjectRef}' is expected.")

FAIL: adminvdc_nsxt_test.go:19: TestVCD.Test_CreateNsxtOrgVdc

Deserialization works, but serialization isn't. Investigating how to fix

[ArnoSen]

It looks related to https://github.com/golang/go/issues/9519. What I understand is that with respect to namespace prefixes there is an asymmetry between marshaling and unmarshaling. I am looking into this as well.

[vbauzys]

It looks related to golang/go#9519. What I understand is that with respect to namespace prefixes there is an asymmetry between marshaling and unmarshaling. I am looking into this as well.

yes, that's the issue

[ArnoSen]

I made a new commit in the pull request branch. I added a marshal test. After adding a custom XMLMarshaler for AdminVdc where the tag of ResourcePoolRefs is dynamically changed to include the namespace prefix, both the Marshal and Unmarshal test work.

Hopefully this passes the integration test.

[ArnoSen]

(Rebased the pull request branch with this main branch because it was behind)

[vbauzys]

Index: govcd/adminvdc_nsxt_test.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/govcd/adminvdc_nsxt_test.go b/govcd/adminvdc_nsxt_test.go
--- a/govcd/adminvdc_nsxt_test.go   (revision f102e77bcd1a4212a7bc20f4b608d254701e61b4)
+++ b/govcd/adminvdc_nsxt_test.go   (date 1656414029841)
@@ -11,8 +11,8 @@

 import (
    "fmt"
-
    "github.com/vmware/go-vcloud-director/v2/types/v56"
+   "github.com/vmware/go-vcloud-director/v2/util"
    . "gopkg.in/check.v1"
 )

@@ -110,8 +110,8 @@
        vdcConfiguration.ComputeCapacity[0].Memory.Units = "MB"

        vdc, err = adminOrg.CreateOrgVdc(vdcConfiguration)
-       check.Assert(vdc, NotNil)
        check.Assert(err, IsNil)
+       check.Assert(vdc, NotNil)

        AddToCleanupList(vdcConfiguration.Name, "vdc", vcd.org.Org.Name, check.TestName())

@@ -121,7 +121,11 @@
        check.Assert(vdc.Vdc.Name, Equals, vdcConfiguration.Name)
        check.Assert(vdc.Vdc.IsEnabled, Equals, vdcConfiguration.IsEnabled)
        check.Assert(vdc.Vdc.AllocationModel, Equals, vdcConfiguration.AllocationModel)
-       check.Assert(len(adminVdc.AdminVdc.ResourcePoolRefs) > 0, Equals, true)
+       util.Logger.Println(adminVdc.AdminVdc.ResourcePoolRefs)
+       check.Assert(len(adminVdc.AdminVdc.ResourcePoolRefs.VimObjectRefs) > 0, Equals, true)
+       check.Assert(adminVdc.AdminVdc.ResourcePoolRefs.VimObjectRefs[0].VimServerRef, NotNil)
+       check.Assert(adminVdc.AdminVdc.ResourcePoolRefs.VimObjectRefs[0].MoRef, NotNil)
+       check.Assert(adminVdc.AdminVdc.ResourcePoolRefs.VimObjectRefs[0].VimObjectType, NotNil)

        // Test  update
        adminVdc.AdminVdc.Description = "updated-description" + check.TestName()
Index: govcd/adminvdc.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/govcd/adminvdc.go b/govcd/adminvdc.go
--- a/govcd/adminvdc.go (revision f102e77bcd1a4212a7bc20f4b608d254701e61b4)
+++ b/govcd/adminvdc.go (date 1656413950414)
@@ -7,11 +7,10 @@
 import (
    "errors"
    "fmt"
-   "net/http"
-   "net/url"
-
    "github.com/vmware/go-vcloud-director/v2/types/v56"
    "github.com/vmware/go-vcloud-director/v2/util"
+   "net/http"
+   "net/url"
 )

 type AdminVdc struct {
@@ -297,6 +296,7 @@
 func updateVdcAsyncV97(adminVdc *AdminVdc) (Task, error) {
    util.Logger.Printf("[TRACE] updateVdcAsyncV97 called %#v", *adminVdc)
    adminVdc.AdminVdc.Xmlns = types.XMLNamespaceVCloud
+   adminVdc.AdminVdc.XmlnsVmext = types.XMLNamespaceExtension

    // Return the task
    return adminVdc.client.ExecuteTaskRequest(adminVdc.AdminVdc.HREF, http.MethodPut,
Index: types/v56/types.go
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/types/v56/types.go b/types/v56/types.go
--- a/types/v56/types.go    (revision f102e77bcd1a4212a7bc20f4b608d254701e61b4)
+++ b/types/v56/types.go    (date 1656414287021)
@@ -9,6 +9,8 @@
 import (
    "encoding/xml"
    "fmt"
+   "github.com/vmware/go-vcloud-director/v2/util"
+   "reflect"
    "sort"
 )

@@ -444,23 +446,24 @@
 // Description: Represents the admin view of an organization VDC.
 // Since: 0.9
 type AdminVdc struct {
-   Xmlns string `xml:"xmlns,attr"`
+   Xmlns      string `xml:"xmlns,attr"`
+   XmlnsVmext string `xml:"xmlns:vmext,attr,omitempty"`
    Vdc

-   VCpuInMhz2                    *int64         `xml:"VCpuInMhz2,omitempty"`
-   ResourceGuaranteedMemory      *float64       `xml:"ResourceGuaranteedMemory,omitempty"`
-   ResourceGuaranteedCpu         *float64       `xml:"ResourceGuaranteedCpu,omitempty"`
-   VCpuInMhz                     *int64         `xml:"VCpuInMhz,omitempty"`
-   IsThinProvision               *bool          `xml:"IsThinProvision,omitempty"`
-   NetworkPoolReference          *Reference     `xml:"NetworkPoolReference,omitempty"`
-   ProviderVdcReference          *Reference     `xml:"ProviderVdcReference"`
-   ResourcePoolRefs              []VimObjectRef `xml:"ResourcePoolRefs,omitempty"`
-   UsesFastProvisioning          *bool          `xml:"UsesFastProvisioning,omitempty"`
-   OverCommitAllowed             bool           `xml:"OverCommitAllowed,omitempty"`
-   VmDiscoveryEnabled            bool           `xml:"VmDiscoveryEnabled,omitempty"`
-   IsElastic                     *bool          `xml:"IsElastic,omitempty"`                     // Supported from 32.0 for the Flex model
-   IncludeMemoryOverhead         *bool          `xml:"IncludeMemoryOverhead,omitempty"`         // Supported from 32.0 for the Flex model
-   UniversalNetworkPoolReference *Reference     `xml:"UniversalNetworkPoolReference,omitempty"` // Reference to a universal network pool
+   VCpuInMhz2                    *int64           `xml:"VCpuInMhz2,omitempty"`
+   ResourceGuaranteedMemory      *float64         `xml:"ResourceGuaranteedMemory,omitempty"`
+   ResourceGuaranteedCpu         *float64         `xml:"ResourceGuaranteedCpu,omitempty"`
+   VCpuInMhz                     *int64           `xml:"VCpuInMhz,omitempty"`
+   IsThinProvision               *bool            `xml:"IsThinProvision,omitempty"`
+   NetworkPoolReference          *Reference       `xml:"NetworkPoolReference,omitempty"`
+   ProviderVdcReference          *Reference       `xml:"ProviderVdcReference"`
+   ResourcePoolRefs              ResourcePoolRefs `xml:"ResourcePoolRefs,omitempty"`
+   UsesFastProvisioning          *bool            `xml:"UsesFastProvisioning,omitempty"`
+   OverCommitAllowed             bool             `xml:"OverCommitAllowed,omitempty"`
+   VmDiscoveryEnabled            bool             `xml:"VmDiscoveryEnabled,omitempty"`
+   IsElastic                     *bool            `xml:"IsElastic,omitempty"`                     // Supported from 32.0 for the Flex model
+   IncludeMemoryOverhead         *bool            `xml:"IncludeMemoryOverhead,omitempty"`         // Supported from 32.0 for the Flex model
+   UniversalNetworkPoolReference *Reference       `xml:"UniversalNetworkPoolReference,omitempty"` // Reference to a universal network pool
 }

 // VdcStorageProfileConfiguration represents the parameters to assign a storage profile in creation of organization vDC.
@@ -2526,15 +2529,30 @@
    Name string `xml:"name,attr,omitempty"`
 }

+type ResourcePoolRefs struct {
+   VimObjectRefs []VimObjectRefForAdminVdc `xml:"VimObjectRef"`
+}
+
+// Type: VimObjectRefType
+// Namespace: http://www.vmware.com/vcloud/extension/v1.5
+// https://vdc-repo.vmware.com/vmwb-repository/dcr-public/7a028e78-bd37-4a6a-8298-9c26c7eeb9aa/09142237-dd46-4dee-8326-e07212fb63a8/doc/doc/types/VimObjectRefsType.html
+// Description: Represents the Managed Object Reference (MoRef) and the type of a vSphere object.
+// Since: 0.9
+type VimObjectRefForAdminVdc struct {
+   VimServerRef  *Reference `xml:"VimServerRef"`
+   MoRef         string     `xml:"MoRef"`
+   VimObjectType string     `xml:"VimObjectType"`
+}
+
 // Type: VimObjectRefType
 // Namespace: http://www.vmware.com/vcloud/extension/v1.5
 // https://vdc-repo.vmware.com/vmwb-repository/dcr-public/7a028e78-bd37-4a6a-8298-9c26c7eeb9aa/09142237-dd46-4dee-8326-e07212fb63a8/doc/doc/types/VimObjectRefsType.html
 // Description: Represents the Managed Object Reference (MoRef) and the type of a vSphere object.
 // Since: 0.9
 type VimObjectRef struct {
-   VimServerRef  *Reference `xml:"VimObjectRef>VimServerRef"`
-   MoRef         string     `xml:"VimObjectRef>MoRef"`
-   VimObjectType string     `xml:"VimObjectRef>VimObjectType"`
+   VimServerRef  *Reference `xml:"VimServerRef"`
+   MoRef         string     `xml:"MoRef"`
+   VimObjectType string     `xml:"VimObjectType"`
 }

 // Type: VimObjectRefsType
@@ -3042,3 +3060,32 @@
    ExpiresIn    int         `json:"expires_in"`
    RefreshToken interface{} `json:"refresh_token"`
 }
+
+func (adminVdc AdminVdc) MarshalXML(e *xml.Encoder, start xml.StartElement) error {
+   //change the tags through reflection
+
+   value := reflect.ValueOf(adminVdc)
+   valueType := value.Type()
+   structFields := make([]reflect.StructField, 0)
+   util.Logger.Println("11111111111111111")
+   for i := 0; i < valueType.NumField(); i++ {
+       util.Logger.Println(valueType.Field(i).Tag)
+       structFields = append(structFields, valueType.Field(i))
+       if valueType.Field(i).Name == "VimObjectRef" {
+           structFields[i].Tag = `xml:"vmext:VimObjectRef"`
+       }
+       if valueType.Field(i).Name == "VimServerRef" {
+           structFields[i].Tag = `xml:"vmext:VimServerRef"`
+       }
+       if valueType.Field(i).Name == "MoRef" {
+           structFields[i].Tag = `xml:"vmext:MoRef"`
+       }
+       if valueType.Field(i).Name == "VimObjectType" {
+           structFields[i].Tag = `xml:"vmext:VimObjectType"`
+       }
+   }
+   newType := reflect.StructOf(structFields)
+   newValue := value.Convert(newType)
+
+   return e.EncodeElement(newValue.Interface(), start)
+}

These changes should be enough if the Marshalling override would work... I have to switch to other things and will continue later. If this won't work, then two AdminVdc structures are needed. One for marshalling and second for unmarshalling

[ArnoSen]

I improved the marhshaling of AdminVdc and I also added custom marshaling for VimObjectRef. Now the marshaling of an AdminVdc is exactly the same as the XML that I received on my environment when getting a AdminVdc. This is demonstrated in the test TestAdminVDCResourcePoolSerialization/Marshal

The advantage of using custom marshalers is that there will only be one AdminVdc to maintain. I think this is preferred above having an object for marshaling and one for unmarshaling.

[ArnoSen]

I am not sure what is required from me now to move this forward. Please let me know if I can/should do anything to process the PR's merging.

[lvirbalas]

Approval from @Didainius is still needed (he's away for a few weeks).

[ArnoSen]

Approval from @Didainius is still needed (he's away for a few weeks).

Ah, thanks.

[Didainius]

@ArnoSen , I am going to spend time on this now. Sorry, it stayed long, but we need a working solution as it still breaks the test (most probably due to Go mishandling XML as you discussed before), but we need a working solution.

[Didainius]

@ArnoSen , After experimentation, I have made another PR #494. Would this solution fit your needs given this is a read-only field in VCD?

[Didainius]

@ArnoSen , I am closing this in favor of #494. Please have a look and share any comments if possible.

[ArnoSen]

@Didainius I was on holiday for a few weeks so I was not able to respond. I took a look at the new PR and it looks good. Before my holiday I did find some other cases where unmarshaling was not working as expected. If I find these situations again, I will create issues for them.

[Didainius]

@Didainius I was on holiday for a few weeks so I was not able to respond. I took a look at the new PR and it looks good. Before my holiday I did find some other cases where unmarshaling was not working as expected. If I find these situations again, I will create issues for them.

Thanks. Yes - there are some edge cases as usual. Tackling them one by one. In this case it should be good enough to still have it working with this Go XML lib issue. When my team members have a look - we will merge #494