feat: Add option to unwrap structs 1 level down#75
feat: Add option to unwrap structs 1 level down#75erezrokah wants to merge 1 commit intocloudquery:mainfrom
Conversation
98d264c to
529b044
Compare
yevgenypats
left a comment
There was a problem hiding this comment.
I like that option but I would implement it in such a way that when you specify withEmbeddedStruct it will get column string so it will unnest only specific field and it will be up to the user which one.
Ah cool that should solve the inconsistency I mentioned but with more manual configuration on the codegen side. |
Not sure I got it? maybe an example of we can do a short sync? |
529b044 to
d9ce555
Compare
| } | ||
|
|
||
| // Unwrap specific struct fields (1 level deep only) | ||
| func WithUnwrapFieldsStructs(fields []string) TableOptions { |
There was a problem hiding this comment.
Handles the use case of:
type test struct {
Properties *Properties
}
| } | ||
|
|
||
| // Unwrap all fields that are embedded structs (1 level deep only) | ||
| func WithUnwrapAllEmbeddedStructs() TableOptions { |
There was a problem hiding this comment.
Handles the use case of:
type test struct {
*Properties
}
Without the need to configure each struct we'd like to unwrap
|
@yevgenypats as discussed via Zoom I separated the cases of unwrapping all embedded structs and unwrapping specific struct fields. See cloudquery/cloudquery@38338c5 for example usage |
|
@yevgenypats added another commit to fix an issue with non embedded fields. When we unwrap those we need to pre-pend the parent field name to the path resolver cf96430 |
cf96430 to
41f228b
Compare
yevgenypats
left a comment
There was a problem hiding this comment.
Can we deprecate the overridefield here? it will make it easier for me to review this one.
41f228b to
fe7939e
Compare
|
Rebased the PR and it should be good for another review |
| if unicode.IsLower(rune(field.Name[0])) { | ||
| continue | ||
| } | ||
| if sliceContains(t.skipFields, field.Name) { |
There was a problem hiding this comment.
@erezrokah why move all those three checks into the isUnwrap ? as those are not really related?
There was a problem hiding this comment.
I'm not sure I understand, those are extracted to ignoreField which is called by addColumnFromField.
shouldUnwrap is return isFieldStruct(field.Type) && (t.unwrapAllEmbeddedStructFields && field.Anonymous || sliceContains(t.structFieldsToUnwrap, field.Name))
146f441 to
0ec642f
Compare
0ec642f to
aa41b3c
Compare
|
@yevgenypats rebased the PR with latest from |
|
Add a few commits on top - #111 and commented on slack |
|
Closing in favor of #111 |
Summary
Adds an option to unwrap fields from embedded structs (1 level down only).
This is common in Azure to have a top level type with another inner struct for the properties.
Currently the properties will get generated as a single JSON column which doesn't make much sense.
Added an option so this is not a breaking change.
See example commit with this change:
cloudquery/cloudquery@3388162
In the old code gen/manually written versions we use several inconsistent approaches:
Looking at the changes generated by this feature unwrapping 1 level down seems like a decent approach, at least for Azure's case
Also looking at the resources we have using the new Azure SDK, it seems it's not needed as they don't have a "Properties" struct embedded into them https://github.com/erezrokah/cloudquery/blob/3388162b00f99dcc98d613356a589a847d3a520b/plugins/source/azure/resources/services/subscriptions/subscriptions.go#L18
Finally even with this change we have some misses due to inconsistencies with the Azure SDK, for example:
https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/services/authorization/mgmt/2015-07-01/authorization#RoleAssignment has
Propertiesas a regular struct, while https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/services/authorization/mgmt/2015-07-01/authorization#RoleDefinition has it as an embedded structUse the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)