Fix problem with code redeclarations by migueleliasweb · Pull Request #436 · oapi-codegen/oapi-codegen

migueleliasweb · 2021-08-29T02:57:23Z

Fixes: #256 #200 and might also fix: #386.

This PR addresses the problems when the generated code contains multiple redeclarations of methods, constants and types.

You can verify this problem with the Docker OpenAPI reference yaml:

# this converts the yaml from v2 to v3 so we can generate code with oapi-codegen
curl "https://converter.swagger.io/api/convert?url=https://docs.docker.com/engine/api/v1.41.yaml" -H "Accept: application/yaml" -o v1.41.yaml

Run the generation with v1.8.2:

oapi-codegen -package openapigen -generate types > types.go

Run staticcheck linter:

staticcheck types.go

Outputs:

types.go 
src/openapigen/types.go:247:2: 	other declaration of SwarmSpecCAConfigExternalCAsProtocolCfssl (compile)
src/openapigen/types.go:252:2: SwarmSpecCAConfigExternalCAsProtocolCfssl redeclared in this block (compile)
src/openapigen/types.go:683:6: 	other declaration of ContainerSummary_Labels (compile)
src/openapigen/types.go:688:6: 	other declaration of ContainerSummary_NetworkSettings_Networks (compile)
src/openapigen/types.go:693:6: ContainerSummary_Labels redeclared in this block (compile)
src/openapigen/types.go:698:6: ContainerSummary_NetworkSettings_Networks redeclared in this block (compile)
src/openapigen/types.go:2679:6: 	other declaration of SwarmSpec_CAConfig_ExternalCAs_Options (compile)
src/openapigen/types.go:2685:6: 	other declaration of SwarmSpecCAConfigExternalCAsProtocol (compile)
src/openapigen/types.go:2689:6: SwarmSpec_CAConfig_ExternalCAs_Options redeclared in this block (compile)
src/openapigen/types.go:2695:6: SwarmSpecCAConfigExternalCAsProtocol redeclared in this block (compile)
src/openapigen/types.go:5452:34: 	other declaration of Get (compile)
src/openapigen/types.go:5460:35: 	other declaration of Set (compile)
src/openapigen/types.go:5468:35: 	other declaration of UnmarshalJSON (compile)
src/openapigen/types.go:5490:34: 	other declaration of MarshalJSON (compile)
src/openapigen/types.go:5505:52: 	other declaration of Get (compile)
src/openapigen/types.go:5513:53: 	other declaration of Set (compile)
src/openapigen/types.go:5521:53: 	other declaration of UnmarshalJSON (compile)
src/openapigen/types.go:5543:52: 	other declaration of MarshalJSON (compile)
src/openapigen/types.go:5558:34: method Get already declared for type ContainerSummary_Labels struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:5566:35: method Set already declared for type ContainerSummary_Labels struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:5574:35: method UnmarshalJSON already declared for type ContainerSummary_Labels struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:5596:34: method MarshalJSON already declared for type ContainerSummary_Labels struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:5611:52: method Get already declared for type ContainerSummary_NetworkSettings_Networks struct{AdditionalProperties map[string]EndpointSettings "json:\"-\""} (compile)
src/openapigen/types.go:5619:53: method Set already declared for type ContainerSummary_NetworkSettings_Networks struct{AdditionalProperties map[string]EndpointSettings "json:\"-\""} (compile)
src/openapigen/types.go:5627:53: method UnmarshalJSON already declared for type ContainerSummary_NetworkSettings_Networks struct{AdditionalProperties map[string]EndpointSettings "json:\"-\""} (compile)
src/openapigen/types.go:5649:52: method MarshalJSON already declared for type ContainerSummary_NetworkSettings_Networks struct{AdditionalProperties map[string]EndpointSettings "json:\"-\""} (compile)
src/openapigen/types.go:6936:49: 	other declaration of Get (compile)
src/openapigen/types.go:6944:50: 	other declaration of Set (compile)
src/openapigen/types.go:6952:50: 	other declaration of UnmarshalJSON (compile)
src/openapigen/types.go:6974:49: 	other declaration of MarshalJSON (compile)
src/openapigen/types.go:6989:49: method Get already declared for type SwarmSpec_CAConfig_ExternalCAs_Options struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:6997:50: method Set already declared for type SwarmSpec_CAConfig_ExternalCAs_Options struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:7005:50: method UnmarshalJSON already declared for type SwarmSpec_CAConfig_ExternalCAs_Options struct{AdditionalProperties map[string]string "json:\"-\""} (compile)
src/openapigen/types.go:7027:49: method MarshalJSON already declared for type SwarmSpec_CAConfig_ExternalCAs_Options struct{AdditionalProperties map[string]string "json:\"-\""} (compile)

# ran the generation with `this branch`:
go run cmd/oapi-codegen/oapi-codegen.go -package openapigen -generate types > types_fixed.go

Run staticcheck linter again:

staticcheck types_fixed.go

Outputs:

clean af

migueleliasweb · 2021-08-29T03:42:01Z

Hi, @popsu @amanbolat ,

Could you check if this fixes your problem?

migueleliasweb · 2021-08-29T03:53:42Z

Hi, @deepmap-marcinr

Could you review this PR? It's a small change and doesn't introduces any breaking changes.

Btw, I'm aware there seems to be a v2 on the works but in the meantime this can help a lot of people that seem to have this issue with redeclarations.

hoshsadiq · 2021-09-01T10:08:26Z

This PR generated the firefly-iii types fine. This does seem to generate the code in a different order, so I'm struggling to find the main differences between the two generated codes, but looking at the original issue it's:

// Which action is necessary for the rule to fire? Use either store-journal or update-journal.
type RuleTrigger string

//...

// RuleTrigger defines model for RuleTrigger.
type RuleTrigger struct {
	// If the trigger is active. Defaults to true.
	Active    *bool      `json:"active,omitempty"`
	CreatedAt *time.Time `json:"created_at,omitempty"`
	Id        *string    `json:"id,omitempty"`

	// Order of the trigger
	Order *int32 `json:"order,omitempty"`

	// When true, other triggers will not be checked if this trigger was triggered. Defaults to false.
	StopProcessing *bool `json:"stop_processing,omitempty"`

	// The type of thing this trigger responds to. A limited set is possible
	Type      RuleTriggerType `json:"type"`
	UpdatedAt *time.Time      `json:"updated_at,omitempty"`

	// The accompanying value the trigger responds to. This value is often mandatory, but this depends on the trigger.
	Value string `json:"value"`
}

Using this PR, the second RuleTrigger is not there. What determines which type is correct? From the code it looks like it could be based on first type found?

These two are referenced like so:

// Rule defines model for Rule.
type Rule struct {
//...
	// Which action is necessary for the rule to fire? Use either store-journal or update-journal.
	Trigger   RuleTrigger   `json:"trigger"`
	Triggers  []RuleTrigger `json:"triggers"`
//...
}

I could be mistaken, but it looks like both types might be correct, one referenced at Trigger and the other at Triggers.

Planning utilise the client soon-ish, so who knows, it may just work.

hoshsadiq · 2021-09-01T10:13:31Z

Oh! Looking at the openapi spec (below, some code removed for brevity), it seems that yes, Trigger is of type string, but Triggers is an object. So both are needed.

    Rule:
      type: object
//...
      properties:
//...
        trigger:
          type: string
          format: string
          example: store-journal
          description: Which action is necessary for the rule to fire? Use either store-journal or update-journal.
          enum:
          - "store-journal"
          - "update-journal"
//...
        triggers:
          type: array
          items:
            $ref: '#/components/schemas/RuleTrigger'
//...
    RuleTrigger:
      type: object
//...
      properties:
        id:
          type: string
          format: string
          example: "2"
          readOnly: true
//...

migueleliasweb · 2021-09-01T10:15:10Z

Hi @hoshsadiq , from your code example the type itself (RuleTrigger) is actually the same but the second one is a slice of that type. I would still expect it to work .

I will have a look at the output diff from master compared to my branch.

migueleliasweb · 2021-09-01T10:19:31Z

@hoshsadiq note that Trigger and Triggers are just properties of the struct. The underlying type is the same.

hoshsadiq · 2021-09-01T11:06:13Z

@migueleliasweb thanks! I'm not sure that's true. See my second comment, looking at the spec, it seems trigger is indeed a string, but triggers is a slice of RuleTrigger which is an object. So both types are needed, but perhaps I guess they would be named differently. Maybe the trigger should not be turned into a type considering it's just a primitive string (or is it because of the enum?).

migueleliasweb · 2021-09-01T11:27:36Z

@see my second comment, looking at the spec, it seems trigger is indeed a string, but triggers is a slice of RuleTrigger which is an object.

I think what happened was I was typing and you've edited your comment. Yeah, I see the different types now.

So in regards the generated code:

I had a quick look at the yaml definition from the link you posted (link) and I could only see one RuleTrigger being used as an array for RuleStore.Triggers. I couldn't find the RuleTrigger as a string property.

I also tried to run locally the code generation and it failed with this msg:

$ go run cmd/oapi-codegen/oapi-codegen.go -generate types /tmp/foo/firefly-iii-1.5.2.yaml > /tmp/foo/out.go
error generating code: error creating operation definitions: error describing global parameters for GET//api/v1/cron/{cliToken}: error generating type for param (force): error resolving primitive type
exit status 1

Maybe you posted the wrong link? Or maybe you're generating the code with a changed local version of that yaml? 🤷‍♂️

Anyway, the more examples we can test this PR the better. Keep it up.

hoshsadiq · 2021-09-01T11:44:17Z

I had a quick look at the yaml definition from the link you posted (link) and I could only see one RuleTrigger being used as an array for RuleStore.Triggers. I couldn't find the RuleTrigger as a string property.

That's right! This property:

        trigger:
          type: string
          format: string
          example: store-journal
          description: Which action is necessary for the rule to fire? Use either store-journal or update-journal.
          enum:
          - "store-journal"
          - "update-journal"

gets generated as:

type RuleTrigger string

Which doesn't seem correct. The struct is correctly generated though.

I also tried to run locally the code generation and it failed with this msg:

Aah yes, the original spec seems to not be fully accepted by oapi-codegen. I ran the following sed on the yaml prior to running it through oapi-codegen: sed -i '/format: (integer|boolean)/d' firefly-iii.yaml. I'm not on the computer I ran this on so it might be slightly different.

migueleliasweb · 2021-09-01T12:34:24Z

I had to make a slight change for the sed command to get it working locally but I got it. It was missing the -E.

Yeah, the output is odd. I reckon I didn't even change anything where the Schema is created/parsed but I will have a look at this.

migueleliasweb · 2021-09-01T13:25:07Z

Ok, I'm 90% certain I've found the problem.

Struct enums are prefixed by the struct name. In your case, the Rule struct with the Trigger enum caused an enum type RuleTrigger to be created but at the same time there is a type already defined with the same name for the Triggers array.

A quick fix is:

$ wget https://api-docs.firefly-iii.org/firefly-iii-1.5.2.yaml
$ sed -i -E '/format:\ (integer|boolean)/d' firefly-iii-1.5.2.yaml
$ sed -i 's/RuleTrigger\:/RuleTrigger2\:/' firefly-iii-1.5.2.yaml
$ sed -i "s/'\#\/components\/schemas\/RuleTrigger'/'\#\/components\/schemas\/RuleTrigger2'/" firefly-iii-1.5.2.yaml

I might also create a PR later for this separately. It could be useful in scenarios like this one to have Enum as a prefix to diferentiate the two types.

hoshsadiq · 2021-09-03T08:09:04Z

That works for me, and yeah I agree, this specific issue I had is probably unrelated to this PR.

deepmap-marcinr · 2021-09-22T17:07:41Z

Thank you for doing this. Do you have time to rebase and resolve conflicts? I'll happily merge it.

migueleliasweb · 2021-09-23T00:33:34Z

Hi @deepmap-marcinr , awesome! I will fix the conflicts and will let you know.

Cheers 🎉

migueleliasweb · 2021-10-04T06:17:34Z

Hi @deepmap-marcinr ,

I've updated this PR and now, for some reason, it's complaining about a change in the generated code that seems unrelated to my changes. (see Travic CI output)

Any ideas?

deepmap-marcinr · 2021-10-04T22:48:41Z

I'll merge and fix.

This should resolve oapi-codegen#386 which is still an issue for clients, the changes in oapi-codegen#436 fixed it for types and server but not clients. This small change enables users to override the suffix where it could clash with existing names.

migueleliasweb force-pushed the fix-duplicate-types branch from eab1942 to ee5ee05 Compare August 29, 2021 04:52

Fix type redeclarations

a6a2909

migueleliasweb force-pushed the fix-duplicate-types branch from a7aba0e to a6a2909 Compare October 4, 2021 06:13

deepmap-marcinr merged commit f979a85 into oapi-codegen:master Oct 4, 2021

wolfeidau mentioned this pull request Dec 6, 2021

fix(response): added an option to override the response suffix #493

Merged

adrianpk pushed a commit to foorester/oapi-codegen that referenced this pull request Jan 16, 2024

Fix type redeclarations (oapi-codegen#436)

6b46e3d

adrianpk added a commit to foorester/oapi-codegen that referenced this pull request May 31, 2024

Fix type redeclarations (oapi-codegen#436)

b516a71

Uh oh!

Conversation

migueleliasweb commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

migueleliasweb commented Aug 29, 2021

Uh oh!

migueleliasweb commented Aug 29, 2021

Uh oh!

hoshsadiq commented Sep 1, 2021

Uh oh!

hoshsadiq commented Sep 1, 2021

Uh oh!

migueleliasweb commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

migueleliasweb commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoshsadiq commented Sep 1, 2021

Uh oh!

migueleliasweb commented Sep 1, 2021

Uh oh!

hoshsadiq commented Sep 1, 2021

Uh oh!

migueleliasweb commented Sep 1, 2021

Uh oh!

migueleliasweb commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hoshsadiq commented Sep 3, 2021

Uh oh!

deepmap-marcinr commented Sep 22, 2021

Uh oh!

migueleliasweb commented Sep 23, 2021

Uh oh!

migueleliasweb commented Oct 4, 2021

Uh oh!

deepmap-marcinr commented Oct 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

migueleliasweb commented Aug 29, 2021 •

edited

Loading

migueleliasweb commented Sep 1, 2021 •

edited

Loading

migueleliasweb commented Sep 1, 2021 •

edited

Loading

migueleliasweb commented Sep 1, 2021 •

edited

Loading