-
-
Notifications
You must be signed in to change notification settings - Fork 433
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
openapi3: support format conditional on doc.openapi
version
#744
base: master
Are you sure you want to change the base?
Conversation
3301b24
to
ab2416b
Compare
@fenollp finally made up with implementing a string format support based on OpenAPI minor version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! Thanks for this. This is going to the right direction. I mostly have comments on the API design.
func (format *Format) add(minMinorVersion uint64, vFormat *versionedFormat, override bool) { | ||
if format != nil { | ||
if format.versionedFormats == nil { | ||
format.versionedFormats = make([]*versionedFormat, minMinorVersion+1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is allocating a lot of unused space. I'd rather you use a dynamically-sized map
. Use an additional slice for ordered map keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did that so it is very fast to determine format to use for a specific minor version when validating.
Since number of minor versions won't be too big, I don't think this can be an memory issue.
return nil | ||
} | ||
|
||
func (format Format) DefinedForMinorVersion(minorVersion uint64) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to determine if we keep it so one can determine if a definition already exists for a specific minor version. Since format does not expose its content, this is the only way for one to know about it.
If we keep, I will need to add tests
openapi3/schema_formats.go
Outdated
|
||
// DefineStringFormatStartingWithOpenAPIMinorVersion defines a new regexp pattern for a given format to apply for a minor version of OpenAPI 3 | ||
// override allows to enforce regex usage to all subsequent minor versions of OpenAPI | ||
func DefineStringFormatStartingWithOpenAPIMinorVersion(name string, fromMinorVersion uint64, pattern string, override bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why uint64
? uint8
or even uint16
is large enough.
I like the idea but I'd rather have a simpler API:
- a shorter method such as the existing
DefineStringFormat
(extend with variadic args) - make
override=true
the only behavior. API users will simply make multiple calls to cut specific ranges - turn
fromMinorVersion
into afloat
(f32
if for allY
,3.Y
can be expressed and I believe it's a yes) so users can seeDefineStringFormat(name, pattern, 3.1)
. The call should panic on non-nil non-floats and compare minors with a0.4
epsilon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(yes, I'm asking to extend the existing 2 methods with a variadic float)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint64 comes from the string parsing function used when parsing OpenAPI document
=> Go string parsing returns a uint64
=> Casting to uint8 would require minor version
Since most processors are 64 bits, using an uint8/uint64 does save memory nor it brings performance gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gone with the variadic options. have a look
func TestStringFormatsStartingWithOpenAPIMinorVersion(t *testing.T) { | ||
DefineStringFormatStartingWithOpenAPIMinorVersion("test", 0, "test0", false) | ||
for i := uint64(0); i < 10; i++ { | ||
if assert.Contains(t, SchemaStringFormats, "test") && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prefer the require
package over assert
. This makes for fewer nesting.
@@ -18,6 +18,8 @@ type schemaValidationSettings struct { | |||
defaultsSet func() | |||
|
|||
customizeMessageError func(err *SchemaError) string | |||
|
|||
openapiMinorVersion uint64 // defaults to 0 (3.0.z) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint8
is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see discussion earlier
} | ||
|
||
// MinorVersion returns minor version from string assuming 0 is the default | ||
func (oai Version) Minor() uint64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint64
seems too big. Also why not return the two possible errors? (or edit func doc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you introduced this for use by the schema validation settings. Let's return a float then :)
@@ -3,18 +3,21 @@ const TypeArray = "array" ... | |||
const FormatOfStringForUUIDOfRFC4122 = `^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$` ... | |||
const SerializationSimple = "simple" ... | |||
var SchemaErrorDetailsDisabled = false ... | |||
var ErrInvalidVersion ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your rationale to export this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So one can check at loading time he got an invalid error version, not some other kind of error.
doc.openapi
version
@cboitel Hey there. Do you need help moving this forward? |
Nope. Just missed it while I was on vacations. Will get back to it next week. |
6855e99
to
bb0e0e5
Compare
I had a hard time to rebase. Finally found it was related to some new/existing tests defining string formats for uuid but not restoring string formats to original value. I created a new Save/Restore/RestoreDefault functions for string formats so one can easily do that like we need in our tests. I will now address your earlier comments in the upcoming days. |
eaa7245
to
0e202c6
Compare
0e202c6
to
355d474
Compare
Implements string format improvements in issue #582 with openapi minor version support.