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

Rewrite Accessor to fix array access issues, also add a Delete method #142

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Niels-Be
Copy link

@Niels-Be Niels-Be commented Aug 27, 2023

Summary

This is a rewrite of the accessor code to fix a lot of issues with access to arrays.

I added some tests that were failing bevor this rewrite.

There should be no breaking changes with this code.
However the new code could allow errors to be forwarded to the caller.
e.g. for Get() to differentiate between not found and json null value.
Or to notify the caller about an invalid path expression.

Maybe add new methods for this?

In this PR all errors are just swallowed to keep compatibility.

Additionally a new Delete method (as requested in #141) is introduced which was trivial to add with the new code.

Checklist

  • Tests are passing: task test
  • Code style is correct: task lint

Copy link
Collaborator

@geseq geseq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. It looks generally fine, but I'm quite concerned about the extensive use of reflection and its affect on performance.

@hanzei thoughts?

if nextObj.Kind() == reflect.Interface {
nextObj = nextObj.Elem()
}
// fmt.Printf("key: %s %v\n", key, nextObj.Kind())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fmt.Printf("key: %s %v\n", key, nextObj.Kind())

key := path[index]
arrayIndex := getArrayIndex(key)
var nextObj reflect.Value
// fmt.Printf("current: %s %v\n", key, currentObj.Kind())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fmt.Printf("current: %s %v\n", key, currentObj.Kind())

if nextArrayIndex >= 0 {
// next element will be an array
if !nextObj.IsValid() || nextObj.Kind() != reflect.Slice {
// fmt.Printf("new slice for %s\n", key)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fmt.Printf("new slice for %s\n", key)

// fmt.Printf("new slice for %s\n", key)
newObj = reflect.ValueOf(make([]interface{}, nextArrayIndex+1, max(nextArrayIndex+1, 8)))
} else if nextObj.Len() <= nextArrayIndex {
// fmt.Printf("nextObj %s len %d cap %d\n", key, nextObj.Len(), nextObj.Cap())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fmt.Printf("nextObj %s len %d cap %d\n", key, nextObj.Len(), nextObj.Cap())

break
// next element will be an normal object
if !nextObj.IsValid() || nextObj.Kind() != reflect.Map {
// fmt.Printf("new map for %s\n", key)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fmt.Printf("new map for %s\n", key)

return current
}
if newObj != nextObj || (value != nil && index == lastIndex) {
// fmt.Printf("assign key %s to %v\n", key, newObj)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// fmt.Printf("assign key %s to %v\n", key, newObj)

// arrayAccessRegexString is the regex used to extract the array number
// from the access path
arrayAccessRegexString = `^(.+)\[([0-9]+)\]$`
type notFoundError struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't we just make this

var NotFoundError = errors.New("not found")

idx, err := strconv.ParseUint(key[1:len(key)-1], 10, 64)
if err != nil {
// should not happen, otherwise the path is invalid
panic(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still rather not panic and instead return error here.

@hanzei
Copy link
Collaborator

hanzei commented Aug 29, 2023

@Niels-Be Would you be open on writing benchmarks tests for the affected code to measure the performance impact?

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