grokify/go-ringcentral-client

SyncMessages omits some query params; completely breaks incremental sync (ISync)

gplusplus314 opened this issue · 4 comments

gplusplus314 commented

The SyncMessages func does not correctly handle the following query params:

  • direction
  • messageType
  • syncType

I discovered this issue while trying to perform an incremental sync. The API responded with an error that said my syncToken was invalid. After stepping through with a debugger and tracing around, I landed on the broken generated code.

The issue is that all three of these query params, although singular, are only being included when they pass a type assertion for a slice. For example, this:

if localVarTempParam, localVarOk := localVarOptionals["direction"].([]string); localVarOk

Should be changed to this:

if localVarTempParam, localVarOk := localVarOptionals["direction"].(string); localVarOk

While it's an inconvenience for two of the parameters, it completely breaks incremental sync functionality.

Thanks!

gplusplus314 commented

Upon further investigation, this seems to be a class of errors related to the code generation of enum types (Swagger spec collectionFormat: "multi"). In the swagger spec, whenever we have an enum that gets mapped to a map[string]interface{}, a type assertion for []string is made, when it should be string.

I'm not familiar with the code generation, but if you point me in the right direction, I'd be willing to contribute a more permanent fix.

grokify commented

Generally I do a few things to fix the auto-generated code:

By providing the changes in the Swagger spec and post-generation commands, the changes will persist after rebuilds with an updated spec.

grokify commented

While it's an inconvenience for two of the parameters, it completely breaks incremental sync functionality.

I agree this isn't necessary when there are only two enumerated values, since both values is equivalent to specifying no values. For these, one approach would be to remove collectionType.

While most occurrences only have 2 values, listExtensionPhoneNumbers / extension/phone-number has more:

            collectionFormat: "multi"
            enum:
              - "MainCompanyNumber"
              - "AdditionalCompanyNumber"
              - "CompanyNumber"
              - "DirectNumber"
              - "CompanyFaxNumber"
              - "ForwardedNumber"

I just finished upgrading the SDK to use OpenAPI Generator. It's a fork of Swagger Codegen with a benefit of more frequent releases so we can see about getting support for this in. I'll spend some time looking into this and may be able to provide some info on how to proceed.

You can learn more about the project here:

https://github.com/OpenAPITools/openapi-generator

grokify commented

For this investigation, I'll use listExtensionPhoneNumbers as the example.

Here's the spec:

          -
            name: "usageType"
            in: "query"
            description: "Usage type of the phone number"
            required: false
            type: "array"
            items:
              type: "string"
            collectionFormat: "multi"
            enum:
              - "MainCompanyNumber"
              - "AdditionalCompanyNumber"
              - "CompanyNumber"
              - "DirectNumber"
              - "CompanyFaxNumber"
              - "ForwardedNumber"

Here's the generated code:

type ListExtensionPhoneNumbersOpts struct {
	UsageType optional.Interface
	Page      optional.Int32
	PerPage   optional.Int32
}
	if localVarOptionals != nil && localVarOptionals.UsageType.IsSet() {
		localVarQueryParams.Add("usageType", parameterToString(localVarOptionals.UsageType.Value(), "multi"))
	}

It seems like the desired code would be:

type ListExtensionPhoneNumbersOpts struct {
	UsageType optional.SliceString
	Page      optional.Int32
	PerPage   optional.Int32
}
	if localVarOptionals != nil && localVarOptionals.UsageType.IsSet() {
                for _, s := range localVarOptionals.UsageType.Value() {
			localVarQueryParams.Add("usageType", parameterToString(localVarOptionals.UsageType.Value(), "multi"))
                }
	}

optional is a library by antihax that handles Go's non-null zero values: https://github.com/antihax/optional

Different collection formats are here:

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md