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

fix: reduce for json logic was not working #98

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

brennj
Copy link
Collaborator

@brennj brennj commented Nov 27, 2024

This was a bug brought up internally when trying to create a schema similar to the test case. This is because of a false assumption on my side where I didn't need to account for "temp variables" that are indeed supported in json logic for cases like reduce.

@brennj brennj requested review from ollyd and sandrina-p November 27, 2024 21:05
@@ -399,6 +399,7 @@ function checkRuleIntegrity(
subRule.map((item) => {
const isVar = item !== null && typeof item === 'object' && Object.hasOwn(item, 'var');
if (isVar) {
if (Array.isArray(item.var)) return; // Is an accumlator for reduce and can't be checked.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion

Just wondering if there could ever be an array path which we would want to validate? 🤔
Might be safer to be less permissive: const isReducerVar = operator === 'reduce' && Array.isArray(item.var)
This also makes the code more explicit so we don't need code comments. 🙂

@@ -1,6 +1,6 @@
{
"name": "@remoteoss/json-schema-form",
"version": "0.11.8-beta.0",
"version": "0.11.9-dev.20241127210659",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi! Forgive me, I can't review this, I has been wrapping up my work before going on vacation for the next two weeks. I'll defer to @ollyd here :)

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.

3 participants