Skip to content
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

chore: replace all shorthand tags of mapstruct -> mapstructure #3633

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

spiffcs
Copy link
Contributor

@spiffcs spiffcs commented Jan 29, 2025

Description

When doing #3631 I noticed that syft had some incorrect tags for map-structure that might have been auto completed incorrectly in prior PR. This PR updates the struct tags to use the correct mapstructure

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (improve the developer experience, fix a test flake, etc, without changing the visible behavior of Syft)

@spiffcs spiffcs marked this pull request as ready for review January 29, 2025 22:23
@spiffcs spiffcs requested a review from a team January 30, 2025 16:08
@spiffcs spiffcs self-assigned this Jan 30, 2025
Copy link
Contributor

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

These files under the syft/ package are all part of the syft library API. These do not use mapstructure for anything, which is probably why no one has reported issues with these tags before. The only structs that use mapstructure are the config structs, which all reside next to the command-specific stuff under the top-level cmd/ package, such as the command options structs or the options.

I think having these tags on these structs in the syft/ package at all is confusing and we should remove all the mapstructure and mapstruct tags on these structs so contributors don't keep adding them for no reason and potentially confuse these structs with CLI configuration structs.

Other than that, it doesn't really hurt anything to have these tags, but I would update them to match the json tags or the fields exactly -- some of them have changes to the case of letters (e.g. .AuthorEmail vs Authoremail -- the former would have been used in the absence of an appropriate mapstructure tag, so fixing the tag changes the behavior.)

Signed-off-by: Christopher Phillips <[email protected]>
@spiffcs
Copy link
Contributor Author

spiffcs commented Jan 31, 2025

These files under the syft/ package are all part of the syft library API. These do not use mapstructure for anything, which is probably why no one has reported issues with these tags before. The only structs that use mapstructure are the config structs, which all reside next to the command-specific stuff under the top-level cmd/ package, such as the command options structs or the options.

I think having these tags on these structs in the syft/ package at all is confusing and we should remove all the mapstructure and mapstruct tags on these structs so contributors don't keep adding them for no reason and potentially confuse these structs with CLI configuration structs.

Other than that, it doesn't really hurt anything to have these tags, but I would update them to match the json tags or the fields exactly -- some of them have changes to the case of letters (e.g. .AuthorEmail vs Authoremail -- the former would have been used in the absence of an appropriate mapstructure tag, so fixing the tag changes the behavior.)

I've updated them to match their fields with the correct casing.

I'm not sure about removing them from syft/*:
Here we're using them to help with the advanced decoding from a package.json

if err := json.Unmarshal(b, &authorStr); err == nil {
// successfully parsed as a string, now parse that string into fields
fields := internal.MatchNamedCaptureGroups(authorPattern, authorStr)
if err := mapstructure.Decode(fields, &auth); err != nil {
return fmt.Errorf("unable to decode package.json author: %w", err)
}
} else {
// it's a map that may contain fields of various data types (not just strings)
var fields map[string]interface{}
if err := json.Unmarshal(b, &fields); err != nil {
return fmt.Errorf("unable to parse package.json author: %w", err)
}
if err := mapstructure.Decode(fields, &auth); err != nil {
return fmt.Errorf("unable to decode package.json author: %w", err)
}
}
*a = auth
return nil

We can potentially remove them for the python/swipl case as those structs are not used by any downstream mapstructure.Decode calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants