Skip to content
This repository has been archived by the owner on Mar 16, 2024. It is now read-only.

Fix GenericMap.DeepCopyInto (#1844) #1850

Merged
merged 1 commit into from
Jun 29, 2023
Merged

Conversation

njhale
Copy link
Contributor

@njhale njhale commented Jun 26, 2023

Convert GenericMap to and from JSON on DeepCopyInto. This circumvents
the use of runtime.DeepCopyJSON, which panics when int values
exist in the map.

Addresses #1844

@thedadams
Copy link
Contributor

As I understand, DeepCopyJSON doesn't support int because that type differs based on the platform, but it does support int64.

I understand that changing our use of int to int64 would be a much bigger change, but would that give us an advantage here? Should our ObjectMeta types just always use int64?

Just want to get your thoughts on this.

@njhale
Copy link
Contributor Author

njhale commented Jun 27, 2023

@thedadams thanks for taking a look at this!

As I understand, DeepCopyJSON doesn't support int because that type differs based on the platform, but it does support int64.

yup, that's my read too

I understand that changing our use of int to int64 would be a much bigger change, but would that give us an advantage here? Should our ObjectMeta types just always use int64?

I think that's reasonable. After all, there are quite a few places where we're already forced into explicitly using int64; for example, the limit field of ListOptions is int64.

but for this particular situation -- run args in a GenericMap -- I'm not sure at the moment how we could apply that constraint globally.

@njhale njhale requested a review from tylerslaton June 27, 2023 04:19
@ibuildthecloud
Copy link
Member

The GenericMap has also assumed that the data is only native json types. So for example you can put a struct in there. I'm fine with not allowing int. If we really really want it, we can just copy the deepcopy function and add int. That exact logic I've written tons of time and you will probably find it at least twice somewhere in acorn/baaah already.

I believe the reason int isn't supported is because json.Unmarshal will never produce an int. It only creates int64, float64, json.Number.

Convert GenericMap to and from JSON on DeepCopyInto. This circumvents
the use of runtime.DeepCopyJSON, which panics when int values
exist in the map.

Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
@njhale njhale merged commit 1aa6db8 into acorn-io:main Jun 29, 2023
2 checks passed
@njhale njhale deleted the fix/map-dc branch June 29, 2023 00:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants