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

ApplyInterface does not support int number values #77

Closed
roi-tevel opened this issue Apr 1, 2024 · 7 comments · Fixed by #78
Closed

ApplyInterface does not support int number values #77

roi-tevel opened this issue Apr 1, 2024 · 7 comments · Fixed by #78
Assignees

Comments

@roi-tevel
Copy link

func main() {
	rule := map[string]interface{}{
		"==": []interface{}{
			map[string]interface{}{
				"var": "impact_levels",
			},
			1,
		},
	}

	data := map[string]interface{}{
		"impact_levels": 1,
	}

	output, err := jsonlogic.ApplyInterface(rule, data)
	if err != nil {
		fmt.Println(err)
		return
	}

	fmt.Println(output.(bool))
}

this returns an error, the reason seems to be that isNumber is checking for reflect.Float64, and therefor the int is assumed to be a string.

@diegoholiveira
Copy link
Owner

Not sure if this is a bug since json (and javascript) does not have integer as a native type. So, if you want to use ApplyInterface, make sure that all numbers are float64, as specified into encoding/json docs:

To unmarshal JSON into an interface value, Unmarshal stores one of these in the interface value:

bool, for JSON booleans
float64, for JSON numbers
string, for JSON strings
[]interface{}, for JSON arrays
map[string]interface{}, for JSON objects
nil for JSON null

(source: https://pkg.go.dev/encoding/json)

So, to your code work, just ensure that all numbers are float64:

func TestIssue77(t *testing.T) {
	rule := map[string]interface{}{
		"==": []interface{}{
			map[string]interface{}{
				"var": "impact_levels",
			},
			float64(1),
		},
	}

	data := map[string]interface{}{
		"impact_levels": float64(1),
	}

	output, err := ApplyInterface(rule, data)
	if err != nil {
		t.Fatal(err)
	}

	assert.Equal(t, true, output.(bool))
}

@roi-tevel
Copy link
Author

My problem is that my rule is defined in a yaml:

insights:
  - id: some_insight
    prerequisite_rule:
      "==":
        - var: ".properties.impact_levels"
        - 1.0

it gets unmarshaled to such a struct:

	InsightRule struct {
		ID                string                   `yaml:"id"`
		Prerequisites     interface{}              `yaml:"prerequisite_rule"`
	}

and yaml.unmarshal will make this an int and not a float. so it would be really nice if the package could handle also integers correctly

@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented Apr 3, 2024

@diegoholiveira I quickly attempted to fix this while checking one of the flagd issues (see - open-feature/flagd-testbed#115). As you said, JSON parsing should work normally as Go uses float64 by default so this was not relevant for us.

But anyway, see #78 for a fix proposal for int parsing. I think I covered possible parsing locations. Feel free to modify it if you see the need for more changes

@diegoholiveira
Copy link
Owner

@Kavindu-Dodan LGTM. I'll merge it but probably it would be good to remove the ApplyInterface in the future to avoid problems with types since json (and javascript) have different types than Go (and other languages).

Thanks

@Kavindu-Dodan
Copy link
Contributor

@Kavindu-Dodan LGTM. I'll merge it but probably it would be good to remove the ApplyInterface in the future to avoid problems with types since json (and javascript) have different types than Go (and other languages).

Thanks

Welcome, happy to help you with the PR.

Yeah, agree. I think ApplyInterface should be internal. You can start with a deprecation notice and then make it internal after one or two releases :)

@eltonjr
Copy link

eltonjr commented Sep 24, 2024

Hi, sorry to reopen the discussion here. On the matter of ApplyInterface being deprecated: I have a case scenario where both my rules and my data are already empty interfaces (coming from the filesystem or from http) and I can call ApplyInterface.

To replace it with Apply would mean that I'd have to marshal it to something, pass it to jsonlogic, for jsonlogic to then unmarshal it again. For a high-throughput system this overhead can represent a huge impact on cpu and allocations

@diegoholiveira
Copy link
Owner

@eltonjr since the data is coming from filesystem or http call, rule and data would be already a io.Reader, no? I do think you don't need to parse it before calling the jsonlogic.Apply.

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 a pull request may close this issue.

4 participants