search

stmcginnis / gofish

Gofish is a Golang client library for DMTF Redfish and SNIA Swordfish interaction.
BSD 3-Clause "New" or "Revised" License
211 stars 112 forks source link

Correct HttpBootUri JSON case #292

Closed lzap closed 8 months ago

lzap commented 9 months ago

This patch corrects the JSON name for HttpBootUri to the camel case used in the spec. Turns out it has no effect on tests because (TIL) Go JSON marshaler in stdlib is by default case insensitive:

https://pkg.go.dev/encoding/json#Unmarshal

To unmarshal JSON into a struct, Unmarshal matches incoming object keys to the keys used by Marshal (either the struct field name or its tag), preferring an exact match but also accepting a case-insensitive match. By default, object keys which don't have a corresponding struct field are ignored (see Decoder.DisallowUnknownFields for an alternative).

It is worth mentioning that I haven’t tested this new feature yet on a real hardware and the Redfish specification does not tell (or at least I could not figure it out) if JSON keys are supposed to be case sensitive or not. But it is better to fix this.

Anyway, I want this to be fixed an aligned with other fields. The test actually confirms that Go behaviour, it was previously in a sort of hybrid form HttpBootURI.

Can you tell me more about how to implement reading of AllowableValues for this:

            "BootSourceOverrideTarget@Redfish.AllowableValues": [
                "None",
                "Pxe",
                "Floppy",
                "Cd",
                "Usb",
                "Hdd",
                "BiosSetup",
                "Utilities",
                "Diags",
                "UefiTarget",
                "SDCard",
                "UefiHttp"
            ],

It looks like other code performs some sort of custom decoding. I wonder why is that, my initial idea would be to simple add a new field with custom JSON name. Shall I do the same if I want this feature? Thanks.

stmcginnis commented 9 months ago

This change shouldn't be needed. As you point out, the unmarshalling is case insensitive, so there is no need to explicitly provide a JSON name just to change the casing.

For the AllowableValues, that can be done using an "enum" (in Go, just a custom type with some constants). I wonder if all possible values are defined in the spec though. I haven't looked, so that would just be something to check to make sure there isn't a risk of having a value that wouldn't be defined.

lzap commented 8 months ago

the unmarshalling is case insensitive

Yeah but only on the client side, I wonder if the spec states that servers must also do the same. Anyways, closing thanks.