ginkgobioworks / react-json-schema-form-builder

Visual editor for forms based on json schema, built in React JS
Apache License 2.0
324 stars 83 forks source link

Dependencies break when keys are alphabetized #337

Closed crates closed 2 years ago

crates commented 2 years ago

This is a bug with the Gingko Bioworks form builder, which has already been addressed in the RJSF core code base.

If you have fields where some answers or form keys are dependent on other ones, the order in which the JSON is parsed must be maintained in order for the form data to be read in correctly. However, this is a problem: JSON's data specification is such that the order of the keys shouldn't be relevant in how the data structure is processed.

In other words, if you stringify the JSON, and then parse that string, the keys become alphabetized by default. Reordering JSON keys shouldn't break the way that JSON is processed... however, in your implementation of RJSF, this is exactly what happens. As a result, when instrumenting your form builder in any scenario involving JSON.parse(), an error is introduced.

Thankfully, there is already some code available in the RJSF core code base that could likely be appropriated to resolve the same problem in the Ginkgo Bioworks form builder. Let's start with some bug reproduction data.

Here's the data schema you may use to reproduce this error:

{
    "type":"object",
    "title":"Dependencies Bug Reproduction",
    "required":[

    ],
    "properties":{
      "relation":{
        "enum":[
          "true",
          "false"
        ],
        "type":"string",
        "title":"Is it a plant?",
        "enumNames":[
          "Yes",
          "No"
        ]
      }
    },
    "description":"Described",
    "dependencies":{
      "arc":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "arc":{
                "enum":[
                  "true"
                ]
              },
              "bloSto":{
                "type":"string",
                "title":"Outcome 1",
                "default":"Recommendation 1"
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "arc":{
                "enum":[
                  "false"
                ]
              },
              "search":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Choose yes or no",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              }
            }
          }
        ]
      },
      "graph":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "graph":{
                "enum":[
                  "true"
                ]
              },
              "cos":{
                "type":"string",
                "title":"Outcome 2",
                "default":"Recommendation 2"
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "graph":{
                "enum":[
                  "false"
                ]
              },
              "trans":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Is it an animal?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              }
            }
          }
        ]
      },
      "lrgDt":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "lrgDt":{
                "enum":[
                  "true"
                ]
              },
              "fileSys":{
                "enum":[
                  "fileSystem",
                  "object"
                ],
                "type":"string",
                "title":"Choose your path wisely",
                "enumNames":[
                  "File system",
                  "Object"
                ]
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "lrgDt":{
                "enum":[
                  "false"
                ]
              },
              "binBlo":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Is it a mineral?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              }
            }
          }
        ]
      },
      "binBlo":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "binBlo":{
                "enum":[
                  "true"
                ]
              },
              "bloSto":{
                "type":"string",
                "title":"Outcome 3",
                "default":"Recommendation 3"
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "graph":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Is it a person?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              },
              "binBlo":{
                "enum":[
                  "false"
                ]
              }
            }
          }
        ]
      },
      "smb":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "azu":{
                "type":"string",
                "title":"Outcome 4",
                "default":"Recommendation 4"
              },
              "smb":{
                "enum":[
                  "true"
                ]
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "arc":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Is it bigger than a breadbox?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              },
              "smb":{
                "enum":[
                  "false"
                ]
              }
            }
          }
        ]
      },
      "trans":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "trans":{
                "enum":[
                  "true"
                ]
              },
              "azCaRe":{
                "type":"string",
                "title":"Outcome 5",
                "default":"Recommendation 5"
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "cosDa":{
                "type":"string",
                "title":"Outcome 6",
                "default":"Recommendation 6"
              },
              "trans":{
                "enum":[
                  "false"
                ]
              }
            }
          }
        ]
      },
      "relation":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "sql":{
                "type":"string",
                "title":"Outcome 7",
                "default":"Recommendation 7",
                "description":"We suggest this outcome to you now."
              },
              "relation":{
                "enum":[
                  "true"
                ]
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "relation":{
                "enum":[
                  "false"
                ]
              },
              "semi":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Does it need nutrients to grow?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              }
            }
          }
        ]
      },
      "semi":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "cosDa":{
                "type":"string",
                "title":"Outcome 8",
                "default":"Recommendation 8"
              },
              "semi":{
                "enum":[
                  "true"
                ]
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "smb":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Does it taste like anything good?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              },
              "semi":{
                "enum":[
                  "false"
                ]
              }
            }
          }
        ]
      },
      "search":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "azSrch":{
                "type":"string",
                "title":"Outcome 9",
                "default":"Recommendation 9"
              },
              "search":{
                "enum":[
                  "true"
                ]
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "search":{
                "enum":[
                  "false"
                ]
              },
              "timSerDa":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Would you find it in an average household?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              }
            }
          }
        ]
      },
      "timSerDa":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "timSerDa":{
                "enum":[
                  "true"
                ]
              },
              "timSerIns":{
                "type":"string",
                "title":"Outcome 10",
                "default":"Recommendation 10"
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "lrgDt":{
                "enum":[
                  "true",
                  "false"
                ],
                "type":"string",
                "title":"Is it fun to bring to a party?",
                "enumNames":[
                  "Yes",
                  "No"
                ]
              },
              "timSerDa":{
                "enum":[
                  "false"
                ]
              }
            }
          }
        ]
      },
      "fileSys":{
        "oneOf":[
          {
            "required":[

            ],
            "properties":{
              "dtLkSt":{
                "type":"string",
                "title":"Outcome 11",
                "default":"Recommendation 11"
              },
              "fileSys":{
                "enum":[
                  "fileSystem"
                ]
              }
            }
          },
          {
            "required":[

            ],
            "properties":{
              "bloSto":{
                "type":"string",
                "title":"Outcome 12",
                "default":"Recommendation 12"
              },
              "fileSys":{
                "enum":[
                  "object"
                ]
              }
            }
          }
        ]
      }
    }
  }

Here's a UI schema to go with that:

{
    "ui:order":[
      "relation",
      "sql",
      "semi",
      "smb",
      "azu",
      "arc",
      "search",
      "azSrch",
      "timSerDa",
      "timSerIns",
      "lrgDt",
      "fileSys",
      "binBlo",
      "graph",
      "dtLkSt",
      "bloSto",
      "cos",
      "trans",
      "azCaRe",
      "cosDa"
    ]
  }

If you paste those into the Ginkgo Bioworks form builder demo, you will notice two things right away.

First, the form builder crashes with the following error: TypeError: Cannot read properties of undefined (reading 'dependents')

Second, when previewing the form (which uses RJSF core, and therefore this bug isn't an issue), you will notice that the "Preview Form" tab is rendering the above form just fine.

You can also try pasting the same code into the RJSF Playground, where it works just fine.

When this schema was originally built, the data was entirely the same as it is now. However, the keys arrived in a different order, which allowed the form builder to process this data just fine. Now that the order of the keys has been changed, we are getting this bug introduced because a field is being loaded with dependencies in its "oneOf" implementation, which haven't yet been loaded into memory.

Thankfully, a solution shouldn't be too far away. Have a look at the code starting on line 810 of the utils.js file from RJSF core:

function resolveDependencies(schema, rootSchema, formData) {
  // Drop the dependencies from the source schema.
  let { dependencies = {}, ...resolvedSchema } = schema;
  if ("oneOf" in resolvedSchema) {
    resolvedSchema =
      resolvedSchema.oneOf[
        getMatchingOption(formData, resolvedSchema.oneOf, rootSchema)
      ];
  } else if ("anyOf" in resolvedSchema) {
    resolvedSchema =
      resolvedSchema.anyOf[
        getMatchingOption(formData, resolvedSchema.anyOf, rootSchema)
      ];
  }
  return processDependencies(
    dependencies,
    resolvedSchema,
    rootSchema,
    formData
  );
}
function processDependencies(
  dependencies,
  resolvedSchema,
  rootSchema,
  formData
) {
  // Process dependencies updating the local schema properties as appropriate.
  for (const dependencyKey in dependencies) {
    // Skip this dependency if its trigger property is not present.
    if (formData[dependencyKey] === undefined) {
      continue;
    }
    // Skip this dependency if it is not included in the schema (such as when dependencyKey is itself a hidden dependency.)
    if (
      resolvedSchema.properties &&
      !(dependencyKey in resolvedSchema.properties)
    ) {
      continue;
    }
    const {
      [dependencyKey]: dependencyValue,
      ...remainingDependencies
    } = dependencies;
    if (Array.isArray(dependencyValue)) {
      resolvedSchema = withDependentProperties(resolvedSchema, dependencyValue);
    } else if (isObject(dependencyValue)) {
      resolvedSchema = withDependentSchema(
        resolvedSchema,
        rootSchema,
        formData,
        dependencyKey,
        dependencyValue
      );
    }
    return processDependencies(
      remainingDependencies,
      resolvedSchema,
      rootSchema,
      formData
    );
  }
  return resolvedSchema;
}

function withDependentProperties(schema, additionallyRequired) {
  if (!additionallyRequired) {
    return schema;
  }
  const required = Array.isArray(schema.required)
    ? Array.from(new Set([...schema.required, ...additionallyRequired]))
    : additionallyRequired;
  return { ...schema, required: required };
}

function withDependentSchema(
  schema,
  rootSchema,
  formData,
  dependencyKey,
  dependencyValue
) {
  let { oneOf, ...dependentSchema } = retrieveSchema(
    dependencyValue,
    rootSchema,
    formData
  );
  schema = mergeSchemas(schema, dependentSchema);
  // Since it does not contain oneOf, we return the original schema.
  if (oneOf === undefined) {
    return schema;
  } else if (!Array.isArray(oneOf)) {
    throw new Error(`invalid: it is some ${typeof oneOf} instead of an array`);
  }
  // Resolve $refs inside oneOf.
  const resolvedOneOf = oneOf.map(subschema =>
    subschema.hasOwnProperty("$ref")
      ? resolveReference(subschema, rootSchema, formData)
      : subschema
  );
  return withExactlyOneSubschema(
    schema,
    rootSchema,
    formData,
    dependencyKey,
    resolvedOneOf
  );
}

function withExactlyOneSubschema(
  schema,
  rootSchema,
  formData,
  dependencyKey,
  oneOf
) {
  const validSubschemas = oneOf.filter(subschema => {
    if (!subschema.properties) {
      return false;
    }
    const { [dependencyKey]: conditionPropertySchema } = subschema.properties;
    if (conditionPropertySchema) {
      const conditionSchema = {
        type: "object",
        properties: {
          [dependencyKey]: conditionPropertySchema,
        },
      };
      const { errors } = validateFormData(formData, conditionSchema);
      return errors.length === 0;
    }
  });
  if (validSubschemas.length !== 1) {
    console.warn(
      "ignoring oneOf in dependencies because there isn't exactly one subschema that is valid"
    );
    return schema;
  }
  const subschema = validSubschemas[0];
  const {
    [dependencyKey]: conditionPropertySchema,
    ...dependentSubschema
  } = subschema.properties;
  const dependentSchema = { ...subschema, properties: dependentSubschema };
  return mergeSchemas(
    schema,
    retrieveSchema(dependentSchema, rootSchema, formData)
  );
}

// Recursively merge deeply nested schemas.
// The difference between mergeSchemas and mergeObjects
// is that mergeSchemas only concats arrays for
// values under the "required" keyword, and when it does,
// it doesn't include duplicate values.
export function mergeSchemas(obj1, obj2) {
  var acc = Object.assign({}, obj1); // Prevent mutation of source object.
  return Object.keys(obj2).reduce((acc, key) => {
    const left = obj1 ? obj1[key] : {},
      right = obj2[key];
    if (obj1 && obj1.hasOwnProperty(key) && isObject(right)) {
      acc[key] = mergeSchemas(left, right);
    } else if (
      obj1 &&
      obj2 &&
      (getSchemaType(obj1) === "object" || getSchemaType(obj2) === "object") &&
      key === "required" &&
      Array.isArray(left) &&
      Array.isArray(right)
    ) {
      // Don't include duplicate values when merging
      // "required" fields.
      acc[key] = union(left, right);
    } else {
      acc[key] = right;
    }
    return acc;
  }, acc);
}

It looks like this code may have been designed to solve exactly this bug. We're hoping you can take a look, and potentially incorporate the same code into the Ginkgo Bioworks form builder. Solving this bug is an important ingredient to ensuring that dependencies work even after the keys in any JSON object are reordered.

Let me know if you have any questions, and thanks very much for your diligent efforts on such a valuable tool!

crates commented 2 years ago

@raymond-lam - Any chance we could get someone to triage this issue? It's pretty critical that we get a resolution in order to use this tool.

raymond-lam commented 2 years ago

@crates Sorry about the delay. I just put out a PR to fix this.

raymond-lam commented 2 years ago

@crates The fix should be in v2.9.1

crates commented 2 years ago

@raymond-lam - Thanks VERY much for this fix! It's definitely closer to where it needs to be now, but we're still getting a similar error when trying to add a new form element or section to a form that has dependencies.

So now, we can load the forms that previously crashed on load, but when adding a new item to those, we get: Cannot read properties of undefined (reading 'startsWith')

The error originates from: addElem > addElem > addCardObj > getIdFromElementsBlock

Please let me know if there's anything that can be done to resolve this, or if I should open a new ticket. Thanks again!

crates commented 2 years ago

BTW, this happens when using a form that has nested dependencies like the example I posted above. It only occurs after clicking the green/white plus sign button, then selecting either new form element or form section (either will produce the bug), and then clicking the blue "Create" button.

crates commented 2 years ago

One thing you might consider is to use null propagation operators whenever inspecting properties. (It's more commonly known as an optional chaining operator. Not sure where I picked up the other name.) Doing this will help mitigate a lot of cases where you might otherwise encounter bugs.

For instance, I'm noticing errors around the getIdFromElementsBlock method in formBuilder/utils.js:

        ...names.map((name) => {
          if (name.startsWith(DEFAULT_INPUT_NAME)) {
            const index = name.substring(defaultNameLength, name.length);

You might consider adding question marks before your periods to help with undefined errors:

        ...names.map((name) => {
          if (name?.startsWith(DEFAULT_INPUT_NAME)) {
            const index = name?.substring(defaultNameLength, name.length);

Or, alternatively, specifying a default value:

        ...names.map((name) => {
          const safeName = name || '';
          if (safeName.startsWith(DEFAULT_INPUT_NAME)) {
            const index = safeName.substring(defaultNameLength, safeName.length);

I don't know the library enough to open a pull request, but wanted to suggest those possibilities. Thanks again for all of your excellent work on this library! It's greatly appreciated. =)

crates commented 2 years ago

@raymond-lam - Sorry to keep bugging you. Any chance of this getting a fix soon?

raymond-lam commented 2 years ago

@raymond-lam - Sorry to keep bugging you. Any chance of this getting a fix soon?

I can't promise a timeline but I am looking at this.

raymond-lam commented 2 years ago

I haven't forgotten about this....

crates commented 2 years ago

Glad to hear it! We're showcasing a demo of a solution that uses your form builder this week, but without the bug fix we can't edit previously-built forms that have dependency fields (which for our use case, is all of them). We were wondering if we'd have to rewrite the form portion of our app entirely as a result... But for now we're still holding out for a possible fix.

raymond-lam commented 2 years ago

I can’t guarantee that I’ll have a fix for this by the end of the week. You should assume that I won’t, to be safe.

raymond-lam commented 2 years ago

@crates I think I found the fix. I'm issuing a PR -- hopefully I'll release a new version in the next day or so.

crates commented 2 years ago

You're my hero, homie. Your timing is absolutely impeccable. Thanks a million!

raymond-lam commented 2 years ago

@crates hopefully v2.9.2 fixes this bug!

crates commented 2 years ago

@raymond-lam - Not every FOSS project maintainer I've seen has been on top of things, but you and your team are 100% amazing with these updates. Thanks a million. Everything is working phenomenally. You are a hero!