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 draft support for adding/removing/modifying other data elements in dataProcessWrite #809

Merged

Conversation

ivarne
Copy link
Member

@ivarne ivarne commented Sep 30, 2024

The main goal is to add a method to IDataProcessor that takes an IInstanceDataAccessor and a list of changes and to add methods to IInstanceDataAccessor to add and remove data elements.

Major refactorings

  • New service ModelSerializationService to handle (de)serialisation of json and xml with convenience methods *Storage that looks in applicationmetadata
  • Extend IInstanceDataAccessor with methods to add and remove data elements from the instance
    • These are currently lazy, which means that they return void, and updating storage is deferred till after data processing is complete
  • Extend CachedInstanceDataAccessor with functionality to ensure that nothing has been changed, after a certain point. This is used so that after data processing we can save to storage in parallel with validation without having to worry about validators mutating the models undetected.
  • DataElementId was previously intended to be a thin wrapper around System.Guid, to communicate that this isn't just any guid, but a Guid from a specific database column. Every time I created one it was from a DataElement, and it was often required to get the data type, so I added fields to avoid lookups. Now it is more of an Immutable DataElement. Not sure everybody loves that development.
  • DataClient uses the new ModelSerializationService and IDataClient gets a new GetDataBytes to argument the old GetBinaryData for those who want a clean byte[] that isn't wrapped in a Stream.

Open questions

  • New IDataProcessing interface, or just extend the existing with a new method. What to call the new one in that case?
  • I got some help from ChatGPT to write MemoryAsStream in order to be able to pass ReadOnlyMemory<byte> to functions that expects a stream. I also found an alternative in Windows Developer Toolkit. I'm not sure it is worth including a full toolkit for 14 effective lines of code.

Remaining updates to make old code conform to new patterns

  • Deprecate ModelDeserializer and use ModelSerializationService everywhere.
  • Use IInstanceDataAccessor instead of IDataClient in more places where it makes sense to use the new data-processing interface
    • Instance creation
    • Data POST
    • Data PUT
    • Simpler detection for action implementations
    • Run new data-processing method on file/attachment upload

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 the area/validation Custom validations/validation messages label Sep 30, 2024
@ivarne ivarne self-assigned this Sep 30, 2024
@ivarne ivarne marked this pull request as ready for review September 30, 2024 09:56
Copy link
Contributor

@martinothamar martinothamar left a comment

Choose a reason for hiding this comment

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

IInstanceDataAccessor feels a lot more important now then the name suggest, maybe we should consider a new name before we merge to main?

@ivarne
Copy link
Member Author

ivarne commented Oct 1, 2024

IInstanceDataAccessor feels a lot more important now then the name suggest, maybe we should consider a new name before we merge to main?

Do you have any suggestions?

edit: We decided to split the interface into IInstanceDataAccessor and IInstanceDataMutator which inherits from the former

* Use IInstanceDataMutator for actions
* Return List instead of dictionary for models and validation issues in MultipleDataPatch for improved swagger doc
* New IDataWriteProcessor interface with IInstanceDataMutator and change list to mimik IValidator
* IValidator gets ShouldRunForTask(string taskId) so that
    * Required validators only runs for tasks with layout.
    * Expression validators only runs for tasks with datatypes that has expressions validation files
* Revert DataElementIdentifier to a simple wrapper for Guid (that caches the string version)
@ivarne
Copy link
Member Author

ivarne commented Oct 3, 2024

Merging this now, so that we get back to having only one PR

@ivarne ivarne merged commit 1f75bb8 into feat/multi-data-model-with-validation Oct 3, 2024
@ivarne ivarne deleted the feat/ivarne/multiDataProcessing branch October 3, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/validation Custom validations/validation messages
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants