Skip to content

Commit

Permalink
Merge pull request #410 from NeurodataWithoutBorders/fix-requirements
Browse files Browse the repository at this point in the history 
Correct Neuroconv Schema Inconsistency
  • Loading branch information
@CodyCBakerPhD
CodyCBakerPhD authored Oct 2, 2023
2 parents db23a8c + 717b3a3 commit 9d83abf
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 48 deletions.
4 changes: 3 additions & 1 deletion pyflask/manageNeuroconv/manage_neuroconv.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@ def coerce_schema_compliance_recursive(obj, schema):
coerce_schema_compliance_recursive(value, prop_schema)
elif isinstance(obj, list):
for item in obj:
coerce_schema_compliance_recursive(item, schema.get("items", {}))
coerce_schema_compliance_recursive(
item, schema.get("items", schema if "properties" else {})
) # NEUROCONV PATCH

return obj

Expand Down
105 changes: 60 additions & 45 deletions src/renderer/src/stories/JSONSchemaForm.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import { resolveProperties } from "./pages/guided-mode/data/utils";
import { JSONSchemaInput } from "./JSONSchemaInput";
import { InspectorListItem } from "./preview/inspector/InspectorList";

const selfRequiredSymbol = Symbol();

const componentCSS = `
* {
Expand Down Expand Up @@ -153,7 +155,7 @@ export class JSONSchemaForm extends LitElement {
};
}

#base = [];
base = [];
#nestedForms = {};
tables = {};
#nErrors = 0;
Expand Down Expand Up @@ -205,7 +207,7 @@ export class JSONSchemaForm extends LitElement {

if (props.onStatusChange) this.onStatusChange = props.onStatusChange;

if (props.base) this.#base = props.base;
if (props.base) this.base = props.base;
}

getTable = (path) => {
Expand Down Expand Up @@ -242,8 +244,8 @@ export class JSONSchemaForm extends LitElement {
}

// Track resolved values for the form (data only)
updateData(fullPath, value) {
const path = [...fullPath];
updateData(localPath, value) {
const path = [...localPath];
const name = path.pop();

const reducer = (acc, key) => (key in acc ? acc[key] : (acc[key] = {})); // NOTE: Create nested objects if required to set a new path
Expand All @@ -261,7 +263,7 @@ export class JSONSchemaForm extends LitElement {
resolvedParent[name] = value;
}

if (hasUpdate) this.onUpdate(fullPath, value); // Ensure the value has actually changed
if (hasUpdate) this.onUpdate(localPath, value); // Ensure the value has actually changed
}

#addMessage = (name, message, type) => {
Expand All @@ -271,9 +273,9 @@ export class JSONSchemaForm extends LitElement {
container.appendChild(item);
};

#clearMessages = (fullPath, type) => {
if (Array.isArray(fullPath)) fullPath = fullPath.join("-"); // Convert array to string
const container = this.shadowRoot.querySelector(`#${fullPath} .${type}`);
#clearMessages = (localPath, type) => {
if (Array.isArray(localPath)) localPath = localPath.join("-"); // Convert array to string
const container = this.shadowRoot.querySelector(`#${localPath} .${type}`);

if (container) {
const nChildren = container.children.length;
Expand Down Expand Up @@ -348,15 +350,15 @@ export class JSONSchemaForm extends LitElement {
};

#get = (path, object = this.resolved) => {
// path = path.slice(this.#base.length); // Correct for base path
// path = path.slice(this.base.length); // Correct for base path
return path.reduce((acc, curr) => (acc = acc[curr]), object);
};

#checkRequiredAfterChange = async (fullPath) => {
const path = [...fullPath];
#checkRequiredAfterChange = async (localPath) => {
const path = [...localPath];
const name = path.pop();
const element = this.shadowRoot
.querySelector(`#${fullPath.join("-")}`)
.querySelector(`#${localPath.join("-")}`)
.querySelector("jsonschema-input")
.getElement();
const isValid = await this.triggerValidation(name, element, path, false);
Expand All @@ -367,13 +369,13 @@ export class JSONSchemaForm extends LitElement {
if (typeof path === "string") path = path.split(".");

// NOTE: Still must correct for the base here
if (this.#base.length) {
const base = this.#base.slice(-1)[0];
if (this.base.length) {
const base = this.base.slice(-1)[0];
const indexOf = path.indexOf(base);
if (indexOf !== -1) path = path.slice(indexOf + 1);
}

const resolved = path.reduce((acc, curr) => (acc = acc[curr]), schema);
const resolved = this.#get(path, schema);
if (resolved["$ref"]) return this.getSchema(resolved["$ref"].split("/").slice(1)); // NOTE: This assumes reference to the root of the schema

return resolved;
Expand All @@ -382,8 +384,8 @@ export class JSONSchemaForm extends LitElement {
#renderInteractiveElement = (name, info, required, path = []) => {
let isRequired = required[name];

const fullPath = [...path, name];
const externalPath = [...this.#base, ...fullPath];
const localPath = [...path, name];
const externalPath = [...this.base, ...localPath];

const resolved = this.#get(path, this.resolved);
const value = resolved[name];
Expand All @@ -392,11 +394,11 @@ export class JSONSchemaForm extends LitElement {

if (isConditional && !isRequired)
isRequired = required[name] = async () => {
const isRequiredAfterChange = await this.#checkRequiredAfterChange(fullPath);
const isRequiredAfterChange = await this.#checkRequiredAfterChange(localPath);
if (isRequiredAfterChange) {
return true;
} else {
const linkResults = await this.#applyToLinkedProperties(this.#checkRequiredAfterChange, fullPath); // Check links
const linkResults = await this.#applyToLinkedProperties(this.#checkRequiredAfterChange, localPath); // Check links
if (linkResults.includes(true)) return true;
// Handle updates when no longer required
else return false;
Expand All @@ -405,7 +407,7 @@ export class JSONSchemaForm extends LitElement {

const interactiveInput = new JSONSchemaInput({
info,
path: fullPath,
path: localPath,
value,
form: this,
required: isRequired,
Expand All @@ -425,7 +427,7 @@ export class JSONSchemaForm extends LitElement {

return html`
<div
id=${fullPath.join("-")}
id=${localPath.join("-")}
class="form-section ${isRequired || isConditional ? "required" : ""} ${isConditional
? "conditional"
: ""}"
Expand Down Expand Up @@ -459,6 +461,10 @@ export class JSONSchemaForm extends LitElement {

for (let name in requirements) {
let isRequired = requirements[name];

// // NOTE: Uncomment to block checking requirements inside optional properties
// if (!requirements[name][selfRequiredSymbol] && !resolved[name]) continue; // Do not continue checking requirements if absent and not required

if (typeof isRequired === "function") isRequired = await isRequired.call(this.resolved);
if (isRequired) {
let path = parentPath ? `${parentPath}-${name}` : name;
Expand Down Expand Up @@ -511,7 +517,7 @@ export class JSONSchemaForm extends LitElement {
if (this.ignore.includes(key)) return false;
if (this.showLevelOverride >= path.length) return isRenderable(key, value);
if (required[key]) return isRenderable(key, value);
if (this.#getLink([...this.#base, ...path, key])) return isRenderable(key, value);
if (this.#getLink([...this.base, ...path, key])) return isRenderable(key, value);
if (!this.onlyRequired) return isRenderable(key, value);
return false;
})
Expand Down Expand Up @@ -549,7 +555,7 @@ export class JSONSchemaForm extends LitElement {
#isLinkResolved = async (pathArr) => {
return (
await this.#applyToLinkedProperties((link) => {
const isRequired = this.#isRequired(link.slice((this.#base ?? []).length));
const isRequired = this.#isRequired(link.slice((this.base ?? []).length));
if (typeof isRequired === "function") return !isRequired.call(this.resolved);
else return !isRequired;
}, pathArr)
Expand All @@ -558,8 +564,11 @@ export class JSONSchemaForm extends LitElement {

#isRequired = (path) => {
if (typeof path === "string") path = path.split("-");
// path = path.slice(this.#base.length); // Remove base path
return path.reduce((obj, key) => obj && obj[key], this.#requirements);
// path = path.slice(this.base.length); // Remove base path
const res = path.reduce((obj, key) => obj && obj[key], this.#requirements);

if (typeof res === "object") res = res[selfRequiredSymbol];
return res;
};

#getLinkElement = (externalPath) => {
Expand All @@ -572,15 +581,17 @@ export class JSONSchemaForm extends LitElement {
triggerValidation = async (name, element, path = [], checkLinks = true) => {
const parent = this.#get(path, this.resolved);

const pathToValidate = [...(this.base ?? []), ...path];

const valid =
!this.validateEmptyValues && !(name in parent)
? true
: await this.validateOnChange(name, parent, [...(this.#base ?? []), ...path]);
: await this.validateOnChange(name, parent, pathToValidate);

const fullPath = [...path, name]; // Use basePath to augment the validation
const externalPath = [...this.#base, name];
const localPath = [...path, name]; // Use basePath to augment the validation
const externalPath = [...this.base, name];

const isRequired = this.#isRequired(fullPath);
const isRequired = this.#isRequired(localPath);
let warnings = Array.isArray(valid)
? valid.filter((info) => info.type === "warning" && (!isRequired || !info.missing))
: [];
Expand All @@ -599,7 +610,7 @@ export class JSONSchemaForm extends LitElement {

// Clear old errors and warnings on linked properties
this.#applyToLinkedProperties((path) => {
const internalPath = path.slice((this.#base ?? []).length);
const internalPath = path.slice((this.base ?? []).length);
this.#clearMessages(internalPath, "errors");
this.#clearMessages(internalPath, "warnings");
}, externalPath);
Expand All @@ -613,9 +624,9 @@ export class JSONSchemaForm extends LitElement {
}

// Clear old errors and warnings
this.#clearMessages(fullPath, "errors");
this.#clearMessages(fullPath, "warnings");
this.#clearMessages(fullPath, "info");
this.#clearMessages(localPath, "errors");
this.#clearMessages(localPath, "warnings");
this.#clearMessages(localPath, "info");

const isFunction = typeof valid === "function";
const isValid =
Expand All @@ -632,8 +643,8 @@ export class JSONSchemaForm extends LitElement {
this.checkStatus();

// Show aggregated errors and warnings (if any)
warnings.forEach((info) => this.#addMessage(fullPath, info, "warnings"));
info.forEach((info) => this.#addMessage(fullPath, info, "info"));
warnings.forEach((info) => this.#addMessage(localPath, info, "warnings"));
info.forEach((info) => this.#addMessage(localPath, info, "info"));

if (isValid && errors.length === 0) {
element.classList.remove("invalid");
Expand All @@ -643,7 +654,7 @@ export class JSONSchemaForm extends LitElement {

await this.#applyToLinkedProperties((path, element) => {
element.classList.remove("required", "conditional"); // Links manage their own error and validity states, but only one needs to be valid
}, fullPath);
}, localPath);

if (isFunction) valid(); // Run if returned value is a function

Expand All @@ -661,7 +672,7 @@ export class JSONSchemaForm extends LitElement {
[...path, name]
);

errors.forEach((info) => this.#addMessage(fullPath, info, "errors"));
errors.forEach((info) => this.#addMessage(localPath, info, "errors"));
// element.title = errors.map((info) => info.message).join("\n"); // Set all errors to show on hover

return false;
Expand All @@ -679,7 +690,7 @@ export class JSONSchemaForm extends LitElement {
if (renderable.length === 0) return html`<p>No properties to render</p>`;

let renderableWithLinks = renderable.reduce((acc, [name, info]) => {
const externalPath = [...this.#base, ...path, name];
const externalPath = [...this.base, ...path, name];
const link = this.#getLink(externalPath); // Use the base path to find a link

if (link) {
Expand Down Expand Up @@ -740,7 +751,7 @@ export class JSONSchemaForm extends LitElement {
// Render linked properties
if (entry[isLink]) {
const linkedProperties = info.properties.map((path) => {
const pathCopy = [...path].slice((this.#base ?? []).length);
const pathCopy = [...path].slice((this.base ?? []).length);
const name = pathCopy.pop();
return this.#renderInteractiveElement(name, schema.properties[name], required, pathCopy);
});
Expand All @@ -756,7 +767,7 @@ export class JSONSchemaForm extends LitElement {

const hasMany = renderable.length > 1; // How many siblings?

const fullPath = [...path, name];
const localPath = [...path, name];

if (this.mode === "accordion" && hasMany) {
const headerName = header(name);
Expand All @@ -767,8 +778,10 @@ export class JSONSchemaForm extends LitElement {
results: { ...results[name] },
globals: this.globals?.[name],

mode: this.mode,

onUpdate: (internalPath, value) => {
const path = [...fullPath, ...internalPath];
const path = [...localPath, ...internalPath];
this.updateData(path, value);
},

Expand All @@ -793,13 +806,13 @@ export class JSONSchemaForm extends LitElement {
this.checkAllLoaded();
},
renderTable: (...args) => this.renderTable(...args),
base: fullPath,
base: [...this.base, ...localPath],
});

const accordion = new Accordion({
sections: {
[headerName]: {
subtitle: `${this.#getRenderable(info, required[name], fullPath, true).length} fields`,
subtitle: `${this.#getRenderable(info, required[name], localPath, true).length} fields`,
content: this.#nestedForms[name],
},
},
Expand All @@ -811,7 +824,7 @@ export class JSONSchemaForm extends LitElement {
}

// Render properties in the sub-schema
const rendered = this.#render(info, results?.[name], required[name], fullPath);
const rendered = this.#render(info, results?.[name], required[name], localPath);
return hasMany || path.length > 1
? html`
<div style="margin-top: 40px;">
Expand All @@ -834,7 +847,9 @@ export class JSONSchemaForm extends LitElement {
Object.entries(schema.properties).forEach(([key, value]) => {
if (value.properties) {
let nextAccumulator = acc[key];
if (!nextAccumulator || typeof nextAccumulator !== "object") nextAccumulator = acc[key] = {};
const isNotObject = typeof nextAccumulator !== "object";
if (!nextAccumulator || isNotObject)
nextAccumulator = acc[key] = { [selfRequiredSymbol]: !!nextAccumulator };
this.#registerRequirements(value, requirements[key], nextAccumulator);
}
});
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/src/stories/JSONSchemaInput.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ export class JSONSchemaInput extends LitElement {
(this.onValidate
? this.onValidate()
: this.form
? this.form.validateOnChange(key, parent, fullPath, v)
? this.form.validateOnChange(key, parent, [...this.form.base, ...fullPath], v)
: "")
);
},
Expand Down
2 changes: 1 addition & 1 deletion src/renderer/src/stories/pages/guided-mode/data/GuidedMetadata.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ export class GuidedMetadataPage extends ManagedPage {
this.#checkAllLoaded();
},

onUpdate: (...args) => {
onUpdate: () => {
this.unsavedUpdates = true;
},

Expand Down
8 changes: 8 additions & 0 deletions src/renderer/src/stories/pages/guided-mode/data/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,18 @@ export function resolveProperties(properties = {}, target, globals = {}) {

for (let name in properties) {
const info = properties[name];

// NEUROCONV PATCH: Correct for incorrect array schema
if (info.properties && info.type === "array") {
info.items = { type: "object", properties: info.properties, required: info.required };
delete info.properties;
}

const props = info.properties;

if (!(name in target)) {
if (props) target[name] = {}; // Regisiter new interfaces in results
// if (info.type === "array") target[name] = []; // Auto-populate arrays (NOTE: Breaks PyNWB when adding to TwoPhotonSeries field...)

// Apply global or default value if empty
if (name in globals) target[name] = globals[name];
Expand Down

0 comments on commit 9d83abf

Please sign in to comment.