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

Replace custom reflect.Struct logic with logic from encoding/json #667

Conversation

skirsten
Copy link
Contributor

@skirsten skirsten commented Feb 9, 2023

This PR replaces the existing custom struct reflection logic with the logic from encoding/json.

The code is taken from https://cs.opensource.google/go/go/+/refs/tags/go1.20:src/encoding/json/encode.go;l=1212. It was simplified a bit to remove unneeded code and fields. The core logic is still untouched.

Pros:

  • Standard handling according to Go visibility rules
  • Supports embedded structs
  • Support untagged fields
  • "If it works in encoding/json it should work here" ™️

Cons:

  • Silent errors (just like encoding/json)
    For example, if you supply the same tag twice, the new logic will drop it completely. Luckily, this will be caught by the logic in the upper layers ("Object defines fields not found in struct: ...") so this might not be such a big deal?
  • Can panic (just like encoding/json) if supplied wrong types etc.

There are definitely some trade-offs here. I just needed it to support embedded structs (closes #242) but found that implementing that would be far more difficult than just replacing the logic with the existing logic from the stdlib.

Let me know what you think!

@skirsten skirsten requested a review from a team as a code owner February 9, 2023 23:11
@bflad bflad added enhancement New feature or request reflection Issues and PRs about the reflection subsystem used to convert between attr.Values and Go values. thinking labels Feb 10, 2023
@bflad
Copy link
Member

bflad commented Feb 10, 2023

Hi @skirsten 👋 Thank you very much for this contribution.

To be upfront with expectations: the maintainer team is unfortunately not actively working in this complex area of the codebase in the near future, so I'm not sure when we might get around to properly reloading context in this area's design or reviewing this contribution. Among trying to continually improve the documentation to get the ecosystem migrated away from terraform-plugin-sdk, we already have work queued up for the next two minor version releases (with new major features) that will likely lead into the April or later timeframe.

That said, I can say that any change which introduces more chances of runtime panics will likely not be accepted. I'm not saying that this does because I'm not actively reviewing the code or design here, but wanted to ensure that was mentioned out loud since it may cause the need for approaching potential solutions in a completely different way.

Thanks again.

@drakkan
Copy link

drakkan commented Mar 13, 2023

Hello,

I'm writing a Terraform provider for SFTPGo and embedded structs support will help a lot for example user and group models share a lot of fields and having support for emebedded structs will allow me to easily avoid code duplication (it can also be donw without supporting embedded structs but it will be a bit more complex).

Besides this PR do you plan to support embedded structs?

@bflad
Copy link
Member

bflad commented Mar 14, 2023

Besides this PR do you plan to support embedded structs?

This PR is community-contributed and may not be reviewed or merged for the reasons outlined in #667 (comment). The team that maintains this Go module is not actively designing or implementing changes in the reflection code for at least the next few weeks. I would say we are hopeful that something can be supported for this type of functionality, but cannot make any guarantees at this time while we are focused on other work at the moment. The best place to track discussions and effort updates would likely be the associated issue: #242.

@skirsten skirsten closed this Feb 19, 2024
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request reflection Issues and PRs about the reflection subsystem used to convert between attr.Values and Go values. thinking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Follow" Embedded Structs
3 participants