Remove unused/unnecessary fields.

[rabbah]

I did not find references to these fields. Are these fields present to copy propagate or some other reason?

[mdeuser]

i think those fields were added when the approach was to parse the swagger into Go struct/fields for easier access. it turned out the client never leveraged those fields. since those fields do represent valid swagger coming from an ibm api gateway, there's no harm in keeping them as part of an sdk; however, those extra fields alone do not represent the complete swagger 2.0 specification.

[rabbah]

Right - my concern is that the struct is not complete (or has properties that might not make sense as in the "ibm" labelled ones) and hence is neither helpful or safe since it can drop fields.

[mdeuser]

right now there's no api gw spi, so there is tight coupling with the ibm api gw swagger definition which includes the x-ibm-configuration field that defines the api->action mapping. as an sdk, maybe that struct should be more complete, including all of these peer swagger fields.

[rabbah]

If the struct was treated a json instead there’d be no need for these mappings, is that true? As it is there are no references to these fields and it looks like after some initial validation on this struct - say in create api - the swagger is treated as json blob anyway.

Can you point me to where removing these fields would change functionality in this code so I can understand better.

[mdeuser]

i'm thinking of the client-go as more of an sdk, providing the swagger document in a go accessible struct that clients may or may not fully leverage. the openwhisk-cli happens not to leverage all of these fields, including the ones you've mentioned.

correct, if this were javascript, we would not need to bother with this at all, but since the data is decoded into a struct for certain fields, it's probably best to include all of the peer fields; otherwise there's no way to pull those values from the same struct.

the decoding of the api get response into go structs starts here https://github.com/apache/incubator-openwhisk-client-go/blob/df32dca4aecca4ec30add11ce89eac72928f95f4/whisk/api.go#L424

[rabbah]

I don't understand what you mean by you can't pull values from the same struct. As as json object, you can unmarshall into a map[string, interface] and then read values as json["x-ibm-xyz"].(string) for example for a string type as in the fields I opened the issue about.

[rabbah]

Thanks for the link to the line of code- that is where the json is deserialized into a struct - but there are no references to these fields anywhere in the sdk or the cli. Are you saying other builds of the cli may reference these fields?

[mdeuser]

Are you saying other builds of the cli may reference these fields?

yes. other clients using the client-go sdk could be affected

I don't understand what you mean by you can't pull values from the same struct

once the decode/unmarshalling has completed, the only way to access the data is via the struct definition - even if the original json byte[] had additional json data which was not included in the decode instructions defined by the target struct used to hold the unmarshalled json. if the swagger field reference was originally defined as interface{} or more appropriately map[string]interface{}, then it would have been possible to unmarshal all of the json data into this generic data type. access to all of the fields would be possible, but not without prior knowledge of the desire field labels.

https://blog.golang.org/json-and-go

[rabbah]

You are forcing the unmarshalling into a struct unnecessarily. You can deserialize into map of string to interface and retrieve any priority as I showed above. The mapping to a struct is simply a convenience and in this case not necessary. Further as you see it leaves artifacts that looks like dead fields which from a software engineering perspective is wrong. If there is some client that references a field (eg x-Ibm-xyz and it’s not in this schema you’re out of luck if you insist on these types to be narrow and otherwise we have to pollute the definitions with every possible field that might be used in the future).

The CLI and client should treat the JSON objects opaquely. You are not gaining type safety here. And if your point is simply about accessing fields of a json object individually I pointed above how it can be done. In fact with said approach there would be no need to have x-Ibm-fields except where you need them (which is obviously not in these repos).

[mdeuser]

yep.. understood. i think the options before us are..

1. add additional fields to existing struct. non-breaking.
2. replace the struct with map[string]interface{}. breaking. openwhisk-cli code will require updating to access the json using the generic pattern.
3. remove fields not currently used by the openwhisk-cli client. breaking. openwhisk-cli code will not require updates as it does not currently leverage these fields.
4. leave struct as-is

[rabbah]

Option 1 can lead to proliferation of fields - I don’t like it on engineering principles.

Option 4 is problematic because it has fields that are dead fields from the code we point of view and hence can lead to breakage without one knowing concsequences I’d removing fields. Also I think it doesn’t work if someone is using a swagger that’s generated with differently named fields - which is how I stumbled across these.

I think 2 is the ideal option because it is sound from an engineering practice and offers the maximum flexibility for other clients. But this will need to be done incrementally - because removing for example the Swagger struct will touch every other type!

Does option 3 work? It is progress from the point of view of having no fields that aren’t user but frankly I’m not sure what all the side effects are. You’ve already pointed out possible issues with this.

[mrutkows]

@rabbah @mdeuser As time has passed, and the API (Swagger) has not had much change in its commands/flags/options perhaps to allay concerns of perpetually chasing changes by having to mirror changes here, was wondering if this PR should be closed (option #4 from above) or merged (taking into consideration per concerns of #3 listed above)?

[rabbah]

I still think these flags don't make any sense for the Apache project but I am not actively working on this.