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

Add skipped broken unit test for missing json-logic reduce support #98

Open
wants to merge 3 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
src/jsonLogic.js Outdated
@@ -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. 🙂

package.json Outdated
@@ -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 :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@brennj can what's the status of this PR? If I remember correctly, you were not able to fully fix the bug.

If that's true, could I ask you a favor. Could you update the MR to only have a unit test describing the bug, so we make sure to fix it in the upcoming next/ version. Similar to #107 thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

finally getting around to this, i'm gonna add the test case as a todo as you mention ✍️

@brennj brennj force-pushed the ensure-reduce-is-supported branch from f544251 to d0e402b Compare February 4, 2025 15:39
@brennj brennj changed the title fix: reduce for json logic was not working Add skipped broken unit test for missing json-logic reduce support Feb 4, 2025
@brennj
Copy link
Collaborator Author

brennj commented Feb 4, 2025

rebased with main @dragidavid / @thien-remote with the move to next I'll leave this in your capable hands to ensure this is eventually moved into there. Feel free to merge/disregard this PR!

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