Skip to content

Data model lookups + data model binding validations #1473

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

Merged
merged 40 commits into from
Sep 22, 2023

Conversation

olemartinorg
Copy link
Contributor

Description

Oh boy, this started off simple and then turned into a large undertaking.

To summarize the changes here:

  • I started off wanting to write some tests against real-life apps to verify (and possibly fix) the schema lookup problems @bjosttveit saw in fix(deps): update dependency json-schema-library to v9 #1374. I quickly found out there was a regression in that newer version, and when I looked at failing data models - they didn't really seem too complex. So I started writing a very simple function to try to look up the data model binding path in the model instead of using that library. This grew into SimpleSchemaTraversal.ts
  • At about the same time, I started moving around some code from testUtils.tsx into a dedicated directory to make it easier to see that the code I wrote to read real-life apps (with their layout configuration files, etc) belongs to tests. This work is included here.
  • Also at about the same time, I wrote some code to strip away comments from JSON files and parse them in a more tolerant way than straight JSON.parse(). That way we could actually read the JSON files in all apps, as the backend parser is more tolerant than ours, and it strips away these comments (and trailing commas, etc) before we see it on the frontend, so these apps worked in real-life situations. See Fixing JSON parsing in schema tests #1439. This work was swallowed in here.
  • When I had made a system for validating data model bindings against the schema, there was no logical place to put it, so I opted to add these into the Layout inspector in DevTools. There are now visible warnings next to components that fail layout validation, along with messages displaying the errors. This, in practice, starts the work on Validate and warn about invalid application configuration #648 (and closes the spin-off Validate and warn against dataModelBindings not found in data model #1463)
  • Some apps failed in strange ways, because our getRootElementPath() used to look up the schema and fetch the first property as the obvious real root of the model. That failed for some newer data models, in effect voiding all schema-based validation for these apps. After lots of struggling, I found a way to look up the 'real root' of models that worked for all existing apps. This is a breaking change, because validations should start working for some apps that didn't before.

Related Issue(s)

Verification/QA

  • Manual functionality testing
    • I have tested these changes manually
    • Creator of the original issue (or service owner) has been contacted for manual testing (or will be contacted when released in alpha)
    • No testing done/necessary
  • Automated tests
    • Unit test(s) have been added/updated
    • Cypress E2E test(s) have been added/updated
    • No automatic tests are needed here (no functional changes/additions)
    • I want someone to help me make some tests
  • UU/WCAG (follow these guidelines until we have our own)
    • I have tested with a screen reader/keyboard navigation/automated wcag validator
    • No testing done/necessary (no DOM/visual changes)
    • I want someone to help me perform accessibility testing
  • User documentation @ altinn-studio-docs
    • Has been added/updated
    • No functionality has been changed/added, so no documentation is needed
    • I will do that later/have created an issue
  • Support in Altinn Studio
    • Issue(s) created for support in Studio
    • This change/feature does not require any changes to Altinn Studio
  • Sprint board
    • The original issue (or this PR itself) has been added to the Team Apps project and to the current sprint board
    • I don't have permissions to do that, please help me out
  • Labels
    • I have added a kind/* label to this PR for proper release notes grouping
    • I don't have permissions to add labels, please help me out

Ole Martin Handeland added 30 commits September 7, 2023 16:34
…can read all current layout files and trim any comments/trailing commas/etc
…rprisingly it works for every single (working) app with a schema - I did NOT expect that.
…upport for mocked repeating groups, producing nodes with indexed data model bindings, cleaning the layout, aaaand fixing the test so that it actually fails when the path was not found (I suspect it got wrong because 'undefined' is in any array?)
…test. This makes it painfully obvious that the newer oneOf-based schemas create trouble for us.
…o see if it was hard to do - and I think I've made a much better one. Using the library, we had 194 layout sets with errors, but now we're at ~103 (below 100 after accounting for arrays of types with null). This also tries to give app developers a more informed error about why we couldn't seem to find the path, and it's orders of magnitudes faster (while probably sacrificing some edge-cases by not strictly adhering to the allOf/anyOf/oneOf spec).
…handling (makes the flow much easier to read)
… least one schema that did not expect this.
…rocessed (and possibly faster) way to figure out the location of the data model schema from a pointer/path
… model schema paths, as these may be encoded into JSON pointers in that file
…tching it is deprecated, and it won't work in real apps with multiple data models anyway.
…anguage support came with compromises). Started implementing checks for layout validation for components and data model bindings.
Ole Martin Handeland added 9 commits September 15, 2023 17:52
…also be defined in the application metadata (it somehow wasn't, but it surely would fail later in that case). Rewriting in appMetadata.ts to get rid of the deprecated function, and adding an error to the developer tools when this fails for some apps in v4.
…ly solve how to find the root of the data model, and I finally found one (after about 2 days). Using the IDataType defined in the application metadata file, I can figure out the root of the model by looking at the definitions and finding the sub-type that matches the class name in `classRef`. This works for all currently known apps, and should be a lot more future-proof than just picking the first property. Added a test that helped me prove it by comparing all data model bindings in (hopefully) working apps, and only one of them failed (for a few data model bindings), which I believe is a mistake on the app developers part.
# Conflicts:
#	src/components/presentation/NavBar.test.tsx
#	src/layout/Custom/CustomWebComponent.test.tsx
… all arguments required. This makes the page order API work again
@olemartinorg olemartinorg added the kind/breaking-change Issue/pull request containing a breaking change label Sep 19, 2023
Copy link
Member

@bjosttveit bjosttveit left a comment

Choose a reason for hiding this comment

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

:godmode:

# Conflicts:
#	src/layout/RadioButtons/RadioButtonsContainerComponent.test.tsx
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 24 Code Smells

26.3% 26.3% Coverage
5.5% 5.5% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@olemartinorg olemartinorg merged commit 384f2cb into main Sep 22, 2023
@olemartinorg olemartinorg deleted the chore/data-model-schema-lookups-test branch September 22, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/breaking-change Issue/pull request containing a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate and warn against dataModelBindings not found in data model
2 participants