Skip to content

Conversation

@ivarne
Copy link
Member

@ivarne ivarne commented Apr 29, 2025

Now all data is cleaned for all (backend) validation if AppSettings.RemoveHiddenData == true and IInstanceDataAccessor has a new method GetCleanDataAccessor so that a DataProcessor (or UserAction) can easily get a copy of the data with hidden data removed for calculations or similar.

One key factor here is that we add a source generator to be able to work with data models more efficiently

During self review I tried to split the PR into smaller commits that stands on its own

  • 63018b3 Move Altinn.App.Analyzer from .Core to .Api

  • ecf795d Make changes for more efficient removal of hidden data

    • Introduce IFormDataWrapper
    • Deprecate DataModelWrapper
    • New helpers in IInstanceDataAccessor
  • 1f015ef Add soure generator for IFormDataWrapper implemenations in .Analyzers
    This includes two test projects.

    • SourceGenerator.Tests: Use the generator directly and verify text output
    • SourceGenerator.Integration.Tests: Reference the generator in build process and test the generated code

    This has to be two projects, because it is really hard to debug a source generator when it runs as part of the build process.

  • 67d2df5 Add Clean and Previous InstanceDataAccessors
    This enables DataProcessors to get the clean versions of data models for calculations.

  • df07e13 Clean data before validation if AppSettings.RemoveHiddenData = true

  • 6589b33 Minor fixes

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

@ivarne ivarne added backport-ignore This PR is a new feature and should not be cherry-picked onto release branches feature Label Pull requests with new features. Used when generation releasenotes labels Apr 29, 2025
@ivarne
Copy link
Member Author

ivarne commented Apr 29, 2025

/publish

@github-actions
Copy link

github-actions bot commented Apr 29, 2025

PR release:

⚙️ Building...
✅ Done!

@ivarne
Copy link
Member Author

ivarne commented Apr 29, 2025

/publish

@github-actions
Copy link

github-actions bot commented Apr 29, 2025

PR release:

⚙️ Building...
✅ Done!

@ivarne ivarne force-pushed the feat/cleanDataAccessor branch from 11165d0 to d6c3aff Compare April 30, 2025 13:12
@ivarne ivarne force-pushed the feat/cleanDataAccessor branch from b878697 to 92c03b0 Compare May 7, 2025 06:24
@ivarne
Copy link
Member Author

ivarne commented May 7, 2025

/publish

@github-actions
Copy link

github-actions bot commented May 7, 2025

PR release:

⚙️ Building...
✅ Done!

@ivarne ivarne force-pushed the feat/cleanDataAccessor branch 9 times, most recently from f0f764f to 1f015ef Compare May 12, 2025 12:54
@ivarne ivarne changed the title Feat/clean data accessor Remove hidden data before validation and make clean data available in DataProcessor May 12, 2025
@ivarne ivarne marked this pull request as ready for review May 12, 2025 13:22
@ivarne ivarne requested a review from martinothamar May 12, 2025 13:31
@ivarne
Copy link
Member Author

ivarne commented May 13, 2025

/publish

@github-actions
Copy link

github-actions bot commented May 13, 2025

PR release:

⚙️ Building...
✅ Done!

{
if (dataType.Type != JsonType.Object)
{
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

This branch could emit a diagnostic, all the values in dataTypes should be objects

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I definitely could add more diagnostics. Not sure I want to do that, because diagnostics from incremental source generators is pretty buggy. I wanted to have a diagnostic for not finding "ClassRef" in compilation, and added some extra just because they were easy.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading up on the whole "diagnostics from incremental generators" topic, it seems like there is no intention to have a create story there. Maybe we should go in the opposite direction then? Drop all diagnostics here and rather implement analyzers if we feel it is necessary?

/// <summary>
/// Initializes a new instance of the <see cref="ModelPathNode"/> class.
/// </summary>
public ModelPathNode(
Copy link
Contributor

Choose a reason for hiding this comment

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

It wasn't obvious to me why cSharpName and jsonName could be empty in some cases. That is for the root node right? Maybe we could have static named constructors like CreateRoot and CreateProperty or something. Type hierarchy/union pattern is also an option

Copy link
Member Author

Choose a reason for hiding this comment

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

That is for the root node right?

Yes, it is only for root.

I think the proper solution would be to create two different types. Node(List<Property>) and Property(cSharpName, jsonName, node).

(Yes, I checked and that is what PolyType does have two levels)

Do you think clarity is worth the rewrite?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is worth the rewrite at some point. If we do that now as part of this PR or in the future I'll leave for you to decide 😄

// Arrays have never been supported in data models.
// Just ignore them for now.
// return (UnwrapNullable(arrayTypeSymbol.ElementType), arrayTypeSymbol);
throw new NotSupportedException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this function create diagnostics instead of throwing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, probably, but since 8.0.0 we crash at runtime if anyone has an array in their data model, and nobody complains (probably because they generate models in studio.)

return (UnwrapNullable(collectionTypeSymbol.TypeArguments[0]), namedTypeSymbol);
}

break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it always ICollection<>? Feels a little brittle. We could build a set of collection symbols in an earlier stage and do set lookups instead

Copy link
Member Author

Choose a reason for hiding this comment

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

The proper thing to do would probably be to use whatever System.Text.Json / PolyType does.

I think we should either try to do the simple thing or rely on a library.

@ivarne
Copy link
Member Author

ivarne commented May 15, 2025

/publish

@github-actions
Copy link

github-actions bot commented May 15, 2025

PR release:

⚙️ Building...
✅ Done!

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
64.37% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarQube Cloud

@ivarne
Copy link
Member Author

ivarne commented Jun 19, 2025

/publish

@github-actions
Copy link

github-actions bot commented Jun 19, 2025

PR release:

⚙️ Building...
✅ Done!

ivarne added 21 commits August 6, 2025 13:08
 * Introduce IFormDataWrapper
 * Deprecate DataModelWrapper
 * New helpers in IInstanceDataAccessor
This includes two test projects.
* SourceGenerator.Tests: Use the generator directly and verify text output
* SourceGenerator.Integration.Tests: Reference the generator in build process and test the generated code

This has to be two projects, because it is really hard to debug a source generator when it runs as part of the build process.
This enables DataProcessors to get the clean versions of data models for calculations.
Backend currently have 3 failiure conditions where a data model for a reference is not found.
* It does not exist in applicationmetadata
* No dataElement has yet been created
* The dataType has maxCount != 1

Currently the backend message is
"DataType non-existing not found in applicationmetadata.json", which is more specific so I modified the shared tests.
…th CleanDataAccessor when RemoveHiddenData=true
…rmDataValidator along with NoIncrementalValidation
The generated code could not add indexes to the end
If a List backing a repeating group contains a null, that is highly likely caused by cleaning
the rows
@ivarne ivarne force-pushed the feat/cleanDataAccessor branch from 876c618 to 2ca64b3 Compare August 6, 2025 11:40
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 6, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
24.8% Coverage on New Code (required ≥ 65%)
21.95% Condition Coverage on New Code (required ≥ 65%)

See analysis details on SonarQube Cloud

@ivarne
Copy link
Member Author

ivarne commented Oct 1, 2025

Now that #1421 is merged, there is no more need for this

@ivarne ivarne closed this Oct 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-ignore This PR is a new feature and should not be cherry-picked onto release branches feature Label Pull requests with new features. Used when generation releasenotes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't use runtime reflection to read from data models (use source generation or IL code instead)

2 participants