Skip to content

Duplicated types error handling#91

Open
lukap3 wants to merge 9 commits intowI2L:masterfrom
lukap3:duplicated-types
Open

Duplicated types error handling#91
lukap3 wants to merge 9 commits intowI2L:masterfrom
lukap3:duplicated-types

Conversation

@lukap3
Copy link
Contributor

@lukap3 lukap3 commented Oct 6, 2022

Defining N>1 different types with the same name and generating a schema using them results in the last type "overriding" the rest without any error/warning.

There is a simple solution: using UseFullSchemaNames(true) adds the package name as a prefix which should resolve the conflict (except in cases where the package name is also the same)

However, the issue remains: there is no error for these conflicts and it can result in incorrect schemas being generated without the user's knowledge.

This PR adds an error to the generator for cases when multiple types defined with the same name (even when using full schema names)

import (
"encoding/json"
"fmt"
"github.com/wI2L/fizz/testdata/test_types"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use import aliases in camelCase for the two type packages.

@@ -0,0 +1,53 @@
package test_types
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see these packages outside of testdata/, which is supposed to contain static data/fixtures for the tests, not type definitions for the test code.

This could live in the openapi/ folder, within subfolders.

}

type D struct {
W W
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nitpick, but to convey the intent, I'd rename the fields Winternal, and Wexternal, or something like to indicate that.

@lukap3 lukap3 requested a review from wI2L October 6, 2022 16:07
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #91 (1ea0568) into master (fe54d35) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #91      +/-   ##
==========================================
+ Coverage   94.37%   94.40%   +0.02%     
==========================================
  Files           7        7              
  Lines         978      983       +5     
==========================================
+ Hits          923      928       +5     
  Misses         39       39              
  Partials       16       16              
Impacted Files Coverage Δ
openapi/generator.go 93.98% <100.00%> (+0.04%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@wI2L
Copy link
Owner

wI2L commented Oct 12, 2022

@lukap3 Need a rebase.

@lukap3
Copy link
Contributor Author

lukap3 commented Oct 12, 2022

@wI2L done! thanks

package extra_types

import (
"github.com/wI2L/fizz/openapi/test_types/base_types"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

camelCase import alias here aswell please.

schema := g.newSchemaFromType(rt(new(extraTypes.D)))
assert.NotNil(t, schema)
assert.Len(t, g.Errors(), 1)
assert.Implements(t, (*error)(nil), g.Errors()[0])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also test that the error is a TypeError containing the message encountered duplicated type, where the error.T is effectively extratypes.D ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@lukap3 lukap3 requested a review from wI2L November 29, 2022 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants