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

fix: Use field name in json type schema if json tag is missing #2011

Merged
merged 4 commits into from
Dec 19, 2024

Conversation

erezrokah
Copy link
Member

@erezrokah erezrokah commented Dec 18, 2024

Summary

I'm working on validating some data (internal issue https://github.com/cloudquery/cloudquery-issues/issues/2971) using the json type schema and noticed a bug (noticeable on AWS where structs don't have json tags).
We should not use the name transformer to get the name as it defaults to ToSnake when a json tag is missing.
Instead we should mimic the JSON marshaling default behavior to use the field name as is.

A better approach can be to create an instance of the type, marshal it to JSON, then use that for the names. Open to ideas how to do that using reflection, with a caveat that we might need to initialize it using non zero values otherwise those can be omitted


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@erezrokah erezrokah requested a review from a team as a code owner December 18, 2024 21:04
@erezrokah erezrokah requested review from k-rheinheimer and marianogappa and removed request for k-rheinheimer December 18, 2024 21:04
@github-actions github-actions bot added the fix label Dec 18, 2024
@marianogappa
Copy link
Contributor

I think this impacts the behaviour of the jsonflattener transformer plugin. It might be that it's relying on this syntax. Could you run an aws sync with the flattener and see if it stops working first?

@erezrokah
Copy link
Member Author

I think this impacts the behaviour of the jsonflattener transformer plugin. It might be that it's relying on this syntax. Could you run an aws sync with the flattener and see if it stops working first?

I'll do that. Also added an option so plugins can override this. It's required for Azure that doesn't use json tags and implements custom marshaling for all models, example in https://github.com/Azure/azure-sdk-for-go/blob/d9d44c0d1d484769706a833157a34975f9623d17/sdk/resourcemanager/compute/armcompute/models_serde.go#L124

image

@erezrokah
Copy link
Member Author

Also opened Azure/azure-sdk-for-go#23882

@github-actions github-actions bot added fix and removed fix labels Dec 19, 2024
@erezrokah
Copy link
Member Author

erezrokah commented Dec 19, 2024

I'll just verify the jsonflattener then we can release this

@erezrokah
Copy link
Member Author

OK so it changes the nested columns names in AWS with the JSON flattener
image which are now a more accurate representation of the JSON structure:
image

We could make it so the jsonflattener always toSnake the flattened columns (we don't say anything about the casing of the columns in https://www.cloudquery.io/blog/introducing-the-json-flattener-transformer-integration).

Regardless don't think this should block this PR

@kodiakhq kodiakhq bot merged commit 7ca8009 into main Dec 19, 2024
8 checks passed
@kodiakhq kodiakhq bot deleted the fix/type_schema_no_json_tags branch December 19, 2024 17:36
kodiakhq bot pushed a commit that referenced this pull request Dec 19, 2024
🤖 I have created a release *beep* *boop*
---


## [4.72.2](v4.72.1...v4.72.2) (2024-12-19)


### Bug Fixes

* Use field name in json type schema if json tag is missing ([#2011](#2011)) ([7ca8009](7ca8009))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants