Adding basic support for XML structure #328
Conversation
| IngressDate string `json:"in_date" xml:"date,attr"` | ||
| } | ||
|
|
||
| type MyOutputStructWithXmlAttribute struct { |
There was a problem hiding this comment.
I think we should add a test for a struct with just xml attributes. As well as one with json:"-"
There was a problem hiding this comment.
I still need to work on this.
Co-authored-by: ccoVeille <[email protected]>
… add customizable processing order using new struct MetadataParsers
- Resolved `gosec` warnings (G115) by adding `//nolint:gosec` comments to suppress integer overflow checks for `uint64` conversions. - Addressed `gosimple` warning (S1021) by merging variable declaration and assignment for `parsersToRegister`. - Fixed formatting issues detected by `gofumpt`, including unnecessary leading newlines and proper formatting of function declarations.
…cts or slices for XML structure
|
Added a new commit to the PR to fix the structure for Additionally, I detected a bug in the current code when handling embedded structs (see test |
| processXMLStructChildren(params) | ||
| } else if params.Field.Type.Kind() == reflect.Slice { | ||
| elemType := params.Field.Type.Elem() | ||
| if elemType.Kind() == reflect.Struct { |
There was a problem hiding this comment.
This check feels weird. Maybe take a look at dive(). You're doing a bit more here than what dive is doing though.
Feel free to resolve this
There was a problem hiding this comment.
I have a struct like this one:
type Fixture struct {
XMLName xml.Name `xml:"Fixture" json:"-"`
// List of levels in the competition.
Level []struct {
Round []struct {
DateBegin string `xml:"dateBegin,omitempty,attr" json:"dateBegin,omitempty" validate:"omitempty,datePattern"`
// Date of the end of the competition.
DateEnd string `xml:"dateEnd,omitempty,attr" json:"dateEnd,omitempty" validate:"omitempty,datePattern"`
// Order of the round.
Order int `json:"order,omitempty" xml:"order,attr"`
RoundName string `xml:"roundName,attr" json:"roundName,omitempty" validate:"required" description:"Round name" example:"Gameweek 17"`
RoundId int `xml:"roundId,attr" json:"roundId,omitempty" validate:"required,min=1" description:"Round ID in our system" example:"4"`
} `json:"round,omitempty" xml:"Round"`
// Level name.
LevelName string `json:"levelName,omitempty" xml:"levelName1,attr" validate:"required" description:"Name of the phase of tournament" example:"Regular Season"`
// Level ID.
LevelNumber int `json:"levelNumber,omitempty" xml:"levelNumber,attr" validate:"required,min=1" description:"ID of the phase of tournament" example:"1"`
} `json:"level,omitempty" xml:"Level"`
// Competition identity name.
Channel string `xml:"channel,attr" json:"channel,omitempty" validate:"required" description:"Name of channel corresponding to tournament." example:"argentinec"`
// Competition name.
Competition string `xml:"competition,attr" json:"competition,omitempty" validate:"required" description:"Season name" example:"Argentina - Campeonato Primera C Temporada 2024"`
}
which is generating this OpenAPI schema:
"Fixture": {
"description": "Fixture schema",
"properties": {
"channel": {
"description": "Name of channel corresponding to tournament.",
"example": "argentinec",
"nullable": true,
"type": "string",
"xml": {
"attribute": true,
"name": "channel"
}
},
"competition": {
"description": "Season name",
"example": "Argentina - Campeonato Primera C Temporada 2024",
"nullable": true,
"type": "string",
"xml": {
"attribute": true,
"name": "competition"
}
},
"level": {
"items": {
"properties": {
"levelName": {
"type": "string"
},
"levelNumber": {
"type": "integer"
},
"round": {
"items": {
"properties": {
"dateBegin": {
"type": "string"
},
"dateEnd": {
"type": "string"
},
"order": {
"type": "integer"
},
"roundId": {
"type": "integer"
},
"roundName": {
"type": "string"
}
},
"type": "object"
},
"type": "array"
}
},
"type": "object"
},
"nullable": true,
"type": "array",
"xml": {
"name": "Level"
}
}
},
"required": [
"channel",
"competition"
],
"type": "object"
}
As you can see, from "level" to its child elements, there is no additional information—only "type" is present. This is the issue that persists.
I added logs to ParseStructTags to trace how it processes the attribute "LevelName." However, even though it is included in the schema, it is not found:
func (mp *MetadataParsers) ParseStructTags(t reflect.Type, schemaRef *openapi3.SchemaRef) {
if t.Kind() == reflect.Ptr {
t = t.Elem()
}
if t.Kind() != reflect.Struct {
return
}
for i := 0; i < t.NumField(); i++ {
field := t.Field(i)
if field.Name == "levelName" || field.Name == "LevelName" {
slog.Error("======= levelName field detected", "field", field.Name)
}
if field.Anonymous {
....
At this point, I’m out of ideas on how to proceed. In your experience, what steps should I take to identify and fix this bug?
There was a problem hiding this comment.
I'll take a look tomorrow morning as I should have some spare time.
There was a problem hiding this comment.
Sorry won't be til later this week. Day job getting in the way.
There was a problem hiding this comment.
Working with the original code where parseStructTags was used directly and the function included the logic for JSON, XML, example, and validation, I managed to make it work.
this is the code (I added enum_values too, to specify all possible values for field):
// parseStructTags parses struct tags and modifies the schema accordingly.
// t must be a struct type.
// It adds the following struct tags (tag => OpenAPI schema field):
// - description => description
// - example => example
// - json => nullable (if contains omitempty)
// - validate:
// - required => required
// - min=1 => min=1 (for integers)
// - min=1 => minLength=1 (for strings)
// - max=100 => max=100 (for integers)
// - max=100 => maxLength=100 (for strings)
func parseStructTags(t reflect.Type, schemaRef *openapi3.SchemaRef) {
if t.Kind() == reflect.Ptr {
t = t.Elem()
}
if t.Kind() != reflect.Struct {
return
}
for i := range t.NumField() {
field := t.Field(i)
if field.Anonymous {
parseStructTags(field.Type, schemaRef)
continue
}
jsonFieldName := field.Tag.Get("json")
jsonFieldName = strings.Split(jsonFieldName, ",")[0] // remove omitempty, etc
if jsonFieldName == "-" {
continue
}
if jsonFieldName == "" {
jsonFieldName = field.Name
}
if slices.Contains([]string{"XMLName"}, jsonFieldName) {
// Skip XMLName field in struct tags as it is not relevant to OpenAPI
continue
}
if schemaRef.Value.Properties == nil {
schemaRef.Value.Properties = make(map[string]*openapi3.SchemaRef)
}
if _, exists := schemaRef.Value.Properties[jsonFieldName]; !exists {
schemaRef.Value.Properties[jsonFieldName] = &openapi3.SchemaRef{
Value: &openapi3.Schema{},
}
}
property := schemaRef.Value.Properties[jsonFieldName]
propertyCopy := *property
propertyValue := *propertyCopy.Value
// --- This section does the magic. ---
if field.Type.Kind() == reflect.Struct {
propertyValue.Type = &openapi3.Types{openapi3.TypeObject}
if propertyValue.Properties == nil {
propertyValue.Properties = make(map[string]*openapi3.SchemaRef)
}
parseStructTags(field.Type, &openapi3.SchemaRef{Value: &propertyValue})
} else if field.Type.Kind() == reflect.Slice && field.Type.Elem().Kind() == reflect.Struct {
propertyValue.Type = &openapi3.Types{openapi3.TypeArray}
itemSchema := openapi3.NewSchemaRef("", openapi3.NewObjectSchema())
parseStructTags(field.Type.Elem(), itemSchema)
propertyValue.Items = itemSchema
}
propertyValue.XML = &openapi3.XML{}
// Xml attributes
xmlTag, ok := field.Tag.Lookup("xml")
if ok {
xmlTagName := strings.Split(xmlTag, ",")[0]
if xmlTagName == "-" {
continue
}
if field.Type.Kind() == reflect.Struct {
for i := 0; i < field.Type.NumField(); i++ {
childField := field.Type.Field(i)
if childField.Name == "XMLName" && childField.Type == reflect.TypeOf(xml.Name{}) {
xmlTagName = childField.Name
break
}
}
}
if xmlTagName == "" {
xmlTagName = field.Name
}
propertyValue.XML.Name = xmlTagName
if slices.Contains(strings.Split(xmlTag, ","), "attr") {
propertyValue.XML.Attribute = true
}
}
// Example
example, ok := field.Tag.Lookup("example")
if ok {
propertyValue.Example = example
if propertyValue.Type.Is(openapi3.TypeInteger) {
exNum, err := strconv.Atoi(example)
if err != nil {
slog.Warn("Example might be incorrect (should be integer)", "error", err)
}
propertyValue.Example = exNum
}
}
// Emun values
emunValues, ok := field.Tag.Lookup("enum_values")
if ok {
values := strings.Split(emunValues, ",")
propertyValue.Enum = make([]any, len(values))
for i, v := range values {
propertyValue.Enum[i] = v
}
}
// Validation
validateTag, ok := field.Tag.Lookup("validate")
validateTags := strings.Split(validateTag, ",")
if ok && slices.Contains(validateTags, "required") {
schemaRef.Value.Required = append(schemaRef.Value.Required, jsonFieldName)
}
for _, validateTag := range validateTags {
if strings.HasPrefix(validateTag, "min=") {
min, err := strconv.Atoi(strings.Split(validateTag, "=")[1])
if err != nil {
slog.Warn("Min might be incorrect (should be integer)", "error", err)
}
if propertyValue.Type.Is(openapi3.TypeInteger) {
minPtr := float64(min)
propertyValue.Min = &minPtr
} else if propertyValue.Type.Is(openapi3.TypeString) {
//nolint:gosec // disable G115
propertyValue.MinLength = uint64(min)
}
}
if strings.HasPrefix(validateTag, "max=") {
max, err := strconv.Atoi(strings.Split(validateTag, "=")[1])
if err != nil {
slog.Warn("Max might be incorrect (should be integer)", "error", err)
}
if propertyValue.Type.Is(openapi3.TypeInteger) {
maxPtr := float64(max)
propertyValue.Max = &maxPtr
} else if propertyValue.Type.Is(openapi3.TypeString) {
//nolint:gosec // disable G115
maxPtr := uint64(max)
propertyValue.MaxLength = &maxPtr
}
}
}
// Description
description, ok := field.Tag.Lookup("description")
if ok {
propertyValue.Description = description
}
jsonTag, ok := field.Tag.Lookup("json")
if ok {
if strings.Contains(jsonTag, ",omitempty") {
propertyValue.Nullable = true
}
}
propertyCopy.Value = &propertyValue
schemaRef.Value.Properties[jsonFieldName] = &propertyCopy
}
}I will try to apply it to this PR next week, if it’s not done before.
Co-authored-by: Dylan Hitt <[email protected]>
- Corrected the implementation to properly utilize `openapi3.NewArraySchema` for defining array types in OpenAPI schemas. - Ensured that the `Type` is set to "array" and properties are moved to `items` when handling `[]struct`. This fix aligns the schema generation with OpenAPI specifications for arrays.
… library for assertions
Co-authored-by: ccoVeille <[email protected]>
|
closed PR on error |
|
restored commit history |
Co-authored-by: ccoVeille <[email protected]>
|
Looking at it today and tomorrow, PR is pretty big but of good quality from what I've currently read ;) |
| for _, entry := range mp.registeredParsers { | ||
| if entry.Name == name { | ||
| // Remove the existing parser before re-registering | ||
| mp.remove(entry) |
There was a problem hiding this comment.
Error is not checked.
Please run make to check the CI
| registeredParsers []MetadataParserEntry | ||
| parserLock sync.Mutex | ||
| registeredNames map[string]bool |
There was a problem hiding this comment.
From what I've read, registeredNames is a Set that just checks that parsers aren't registered twice. And they are re-declared in MetadataParserEntry.Name, am I right?
Since you just iterate on registeredParsers, can you please simply use a map? registeredParsers map[string] MetadataParserFunction. Unique source of truth + all previous capabilities are still available. Using correct data structures always helps readability and maintainability!
| Parser MetadataParserFunction | ||
| } | ||
|
|
||
| type MetadataParsers struct { |
There was a problem hiding this comment.
Please comment the type so one can easily know what is this struct about.
| mp := NewMetadataParsers() | ||
| mp.Initialize(DefaultParsers) |
There was a problem hiding this comment.
Since you use NewMetadataParsers AND Initialize function, why don't you write something like mp := NewMetadataParsers(DefaultParsers) directly? Since it makes sense in a business point of view, I think it's a correct API.
| mp := &MetadataParsers{ | ||
| registeredParsers: []MetadataParserEntry{ | ||
| {Name: "parser1", Parser: func(params MetadataParserParams) { return }}, | ||
| {Name: "parser2", Parser: func(params MetadataParserParams) { return }}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Here we should declare registeredNames: []string{"parser1", "parser2" isn't it? That's why I propose using maps
| // Verify returned slice matches original registered parsers | ||
| func TestGetRegisteredParsersMatchesOriginal(t *testing.T) { | ||
| mp := &MetadataParsers{ | ||
| registeredParsers: []MetadataParserEntry{ | ||
| {Name: "parser1", Parser: func(params MetadataParserParams) { return }}, | ||
| {Name: "parser2", Parser: func(params MetadataParserParams) { return }}, | ||
| }, | ||
| } | ||
|
|
||
| parsers := mp.GetRegisteredParsers() | ||
|
|
||
| require.NotNil(t, parsers) | ||
| require.Len(t, parsers, 2) | ||
|
|
||
| if !reflect.DeepEqual(parsers, mp.registeredParsers) { | ||
| t.Errorf("Returned parsers do not match the original registered parsers") | ||
| } | ||
| } |
There was a problem hiding this comment.
Since mp.GetRegisteredParsers() is returning the mp.registeredParsers, this test is useless. What do you wanted to test exactly?
| // Reset clears all registered parsers from the slice | ||
| func TestResetClearsRegisteredParsers(t *testing.T) { | ||
| mp := &MetadataParsers{ | ||
| registeredParsers: []MetadataParserEntry{ | ||
| {Name: "parser1", Parser: nil}, | ||
| {Name: "parser2", Parser: nil}, | ||
| }, | ||
| registeredNames: map[string]bool{ | ||
| "parser1": true, | ||
| "parser2": true, | ||
| }, | ||
| } | ||
|
|
||
| mp.Reset() | ||
|
|
||
| require.NotNil(t, mp.registeredParsers) | ||
| require.NotNil(t, mp.registeredNames) | ||
|
|
||
| require.Empty(t, mp.registeredParsers) | ||
| require.Empty(t, mp.registeredNames) | ||
| } |
There was a problem hiding this comment.
It's really good to have a such a big file with so many tests, thank you for that!
I'm a little disappointed that it does not test custom "Metadata parsers"
|
It seems that this PR does several things. If I understand correctly:
So this PR is really cool, I like the idea and it really improves Fuego, thank you for all these efforts! 🥳 But by doing 3 things at a time, it makes the PR really big (more than 800 lines!!!) and that's why it is hard to review and to understand. I also feel that it might be quite heavy for you (this PR is a month old), making changes is not easy and it begins to be quite slow. Because of that, I will skip it for the next release (v0.18.0). I encourage you to iterate quickly with small changes rather than making big PRs, so we will be able to add it in the next release (v0.19.0). If you can't, no problem, we're all volunteers, I understand you might not have time :) |
This pull request introduces basic support for XML tags in OpenAPI documentation, allowing the following features to be defined for schema fields:
The
xml:"customName"struct tag allows specifying a custom name for the XML element.The
xml:"name,attr"struct tag marks a field as an XML attribute.The
xml:"name,wrapped"struct tag enables support for wrapping elements (e.g.,<items><item>...</item></items>).