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

Adding field logic to data package #325

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MbolotSuse
Copy link
Contributor

@MbolotSuse MbolotSuse commented Oct 12, 2023

Added a new Field struct which provides the ability to search through unstructed data (such as the data returned from a json call), and easily add or remove values to this data.

Related to #321.

Overview

Current State

Wrangler provides a data package which provides an easy way to manipulate values of map[string]interface{}. This proviedes functionality to:

  • Get a specific value
  • Put a new value
  • Remove an existing value

This package requires that the path to the value be given as a list of strings, like below:

example := map[string]interface{}{
    "hello": map[string]interface{}{
        "nested": "some value"
    }
}
RemoveValue(example, "hello", "nested") // this will remove the "nested" entry from "hello" in example

Similar functionality can also be used with PutValue and GetValue to insert and get values from a map.

Issues with the Current State

The current state has several issues:

  • There's no testing or documentation, making it difficult for users to know what values are accepted by the functions and difficult to know if changes break the functions
  • The current function(s) don't support adding/removing values from a slice
  • This package is used in a couple of places (rancher, other parts of wrangler, steve), causing it to be difficult to change without introducing regressions (especially given the first point about a lack of documentation and testing)

There's another PR open which attempts to supplement these issues (#321), but there are a few limitations to the PR, namely:

  • It only adds support for slices to the Get function (leading to inconsistencies between get, put, and remove)
  • It supports an imprecise syntax (does "0" mean the "0" key in a map or the 0 index entry of a slice?)
  • There are some docs and tests for the new functionality, but the old remain functions are missing tests (and could probably be better documented).

Fixing some of these issues (like the first and second bullet point) would likely break backward compatibility, so there's only so much that can be done with the current framework.

Changes

This PR introduces a new struct and 3 new functions (GetField, PutField, RemoveField) which aim to mirror the current functionality while providing documentation, testing, and more exact typing.

New Struct

The new struct is a Field. This represents the path in an unstructured map to a resource that users want to Get, Remove, or update. The Field has an immediate path (represented by the Name which is set for map entries and ListIndex which is set for list entries) and a SubField (which indicates that the value at this path is a map or slice, and there are further entries in that value).

Complex paths can be constructed by chaining calls to the Child and Index functions, which will setup the field for the user:

nested := NewNameField("hello").Child("nested")
nestedIndex := NewNameField("hello").Child("nested").Index(0)

New Methods

3 New methods have been added, that aim to behave the same as the existing methods with a few enhancements (namely, they use the new field type and support the use of []any).

  • GetField returns the value for a given field (or an error if the field/data was invalid, or the field was not found).
  • RemoveField removes a given field (key + value for maps, just the entry for slices) and returns the new object, the removed value, and an error (only if the field/data was invalid or the field was not found).
  • PutField puts a value into a given field and returns the new object and an error (only if the field/data was invalid or the field was not found). Will recursively create missing entries.

@brandond
Copy link
Member

brandond commented Oct 12, 2023

I'm curious how these differ from what you can do with metav1.Unstructured

For your example of RemoveValue(example, "hello", "nested") - could this not also be done with RemoveNestedField(example, "hello", "nested") from the unstructured package?

@MbolotSuse
Copy link
Contributor Author

MbolotSuse commented Oct 12, 2023

@brandond The big answer would be slice support - from the docs from that page (for example SetNestedField):

SetNestedField sets the value of a nested field to a deep copy of the value provided. Returns an error if value cannot be set because one of the nesting levels is not a map[string]interface{}.

From testing as well, this appears to return an error if you try and recurse through a slice like:

test := map[string]any{
	"test": []any{"hello"},
}
err := unstructured.SetNestedField(test, "hello2", "test", "0")
fmt.Printf("%+v, err: %v \n", test, err) //prints an error

There's also SetNestedSlice, but that appears to be for setting a slice value, not for navigating through a slice.

In short, this can process through slices, that library you linked doesn't seem to be able to do that.

This being said, I was unaware of that library before, so it's possible that I'm missing something, so happy to discuss further.

Added a new Field struct which provides the ability to search through
unstructed data (such as the data returned from a json call), and easily
add or remove values to this data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants