Multipart form data #329

ivarne · 2023-10-24T11:17:49Z

Work in process

Builds on #328, so temporary the target branch is different

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)

src/Altinn.App.Api/Controllers/DataController.cs

@@ -332,19 +330,19 @@

                if (appLogic == null)
                {
-                    _logger.LogError($"Could not determine if {dataType} requires app logic for application {org}/{app}");
+                    _logger.LogError("Could not determine if {dataType} requires app logic for application {org}/{app}", dataType, org, app);


src/Altinn.App.Api/Controllers/DataController.cs

@@ -332,19 +330,19 @@

                if (appLogic == null)
                {
-                    _logger.LogError($"Could not determine if {dataType} requires app logic for application {org}/{app}");
+                    _logger.LogError("Could not determine if {dataType} requires app logic for application {org}/{app}", dataType, org, app);


src/Altinn.App.Api/Controllers/DataController.cs

@@ -332,19 +330,19 @@

                if (appLogic == null)
                {
-                    _logger.LogError($"Could not determine if {dataType} requires app logic for application {org}/{app}");
+                    _logger.LogError("Could not determine if {dataType} requires app logic for application {org}/{app}", dataType, org, app);


src/Altinn.App.Core/Helpers/JsonHelper.cs

+            catch (Exception e)
+            {
+                logger.LogError(e, "Unable to determine changed fields");
+                return null;
+            }


tjololo

Nice work!

src/Altinn.App.Api/Controllers/StatelessDataController.cs

src/Altinn.App.Core/Features/DataProcessing/NullDataProcessor.cs

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializer.cs

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializerResult.cs

src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs

ivarne

Fikset noen av kommentarene

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializerResult.cs

src/Altinn.App.Core/Infrastructure/Clients/Storage/DataClient.cs

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializer.cs

src/Altinn.App.Api/Controllers/StatelessDataController.cs

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializer.cs

tjololo

One last thing I missed, but looks good. Lets get this from review to test 🙁

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializer.cs

Also ignore CS1998 in apps (warning when not awaiting anything in async hooks)

Co-authored-by: Vemund Gaukstad <[email protected]>

Also add a Dockerfile for running tests on linux

tjololo · 2023-11-29T07:43:41Z

Can we add some test for the bad requests. Like that the first part not having name=dataModel and so on?

olemartinorg · 2023-11-29T13:05:43Z

I tried this against frontend v4/main now, and it's not quite there. Should be the same with v3, as I haven't tested with the form data rewrite in frontend yet.

A feature flag, multiPartSave is expected to be returned in applicationmetadata for frontend to start sending multipart changes. I added this manually in FrontendFeatues.cs when testing.
Frontend sends name="dataModel", not name=dataModel (note the quotes). I manually added some quotes in ModelDeserializer when testing.
The whole data model seems to be returned, not the metadata combined with changedFields, as is expected in v3. Be sure not to conflate this task with Return the full data model to app-frontend after processing a write operation #316, as that's an entirely different beast. Also, returning the entire data model on the top level might not be what we want, as it would be nice to include validation messages etc as well in the future.

src/Altinn.App.Api/Controllers/DataController.cs

For some reason backend returns 303 See Other when datamodel updates are availible. This is a redirect code and crashed the tests as no Location header was set.

test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstance.cs

olemartinorg · 2023-12-04T15:33:23Z

Tested again now, and it's all working against v4/main as far as I can tell! 🙌

tjololo

Have a look at the code smells/codeql warnings, and try to add the last few percentages of testscoverage and we should be good to go as far as I can see

src/Altinn.App.Api/Controllers/DataController.cs

Co-authored-by: Vemund Gaukstad <[email protected]>

sonarcloud · 2023-12-08T13:28:29Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

67.7% Coverage
0.0% Duplication

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

ivarne · 2023-12-19T20:49:49Z

src/Altinn.App.Api/Controllers/StatelessDataController.cs

-            Instance virutalInstance = new Instance() { InstanceOwner = owner };
-            await _dataProcessor.ProcessDataRead(virutalInstance, null, appModel);
+            Instance virtualInstance = new Instance() { InstanceOwner = owner };
+            await ProcessAllDataWrite(virtualInstance, appModel);


@tjololo This was not good!!!

Happens to be OK, as ProcessAllDataWrite actually calls processDataRead 😊 ☺️ I'll fix in the next rewrite that uses json patch

* Revert Multipart form data #329 on PUT endpoint * Add JsonPatch and create a new PATCH endpoint for updating form data * Fix a few SonarCloud warnings * Start work on new validation Improve GetJsonPath(Expression) and fix some sonarcloud issues Improve test coverage Full validation rewrite complete and existing tests work Probably still some issues and test coverage was spoty so more tests needs to be written. Make IInstanceValidator obsolete Add DataType as input to validators Fix a few issues to prepare for tests of expressions Add more tests * Fix sonar cloud issues * Rename to get more consistent naming * Clanup DataType/TaskId to allow * Also comment out KeyedService fetching * Fix more sonarcloud issues * Registrer new validators in DI container * Remove dead code * Adjust interface and add a few tests * Add object? previousData to IDataProcessor * Cleanup and more tests * OptionsSource doesn't really need a label to work. In recent fronted versions, this label can also be an expression, so parsing fails when this is expected to be a string. * Removing the requirement for a 'pageRef' property on Summary components. Frontend doesn't use this anymore, and disallows rendering Summary components with it. * Allowing attachments to be deleted (attachments don't have AppLogic) * ValidationIssue.Severity is an int in json Improve handeling of errors in PATCH endpoint code style fixes * Initialize lists for concistency in frontend after patch also add tests * Fix issues with list initialization and prettify tests * Add test to verify that PATCH initializes arrays (as xml serialization does) * Read json configuration file for api tests * Set empty strings to null in patch endpoint xml serializing does always preserve empty string when using attributes and [XmlText]. * Apply suggestions from code review Co-authored-by: Vemund Gaukstad <[email protected]> * Remove caching logic for CanTaskBeEnded * Improve description on validationsource * Rename ShouldRun to HasRelevantChanges fix style issues * Fix dataProcessorRewriter * Add genericDataProcessor * Fix a few codestyle issues * Making some validator sources match those used on the frontend * Fix tests after validator source name change * Adding support for explicit repeating group components (new in frontend v4) --------- Co-authored-by: Ole Martin Handeland <[email protected]> Co-authored-by: Vemund Gaukstad <[email protected]>

ivarne force-pushed the ivarne/multipart-form-data branch 2 times, most recently from 792ae43 to 319ce56 Compare October 25, 2023 20:45

ivarne marked this pull request as ready for review October 26, 2023 07:09

ivarne requested a review from RonnyB71 October 26, 2023 07:54

ivarne self-assigned this Oct 26, 2023

ivarne added this to the 8.0.0 milestone Oct 26, 2023

ivarne linked an issue Oct 26, 2023 that may be closed by this pull request

Support multipart saving #187

Closed

ivarne requested a review from tjololo October 26, 2023 07:57

ivarne changed the base branch from ivarne/warning-fixes to v8 October 26, 2023 07:58

ivarne changed the base branch from v8 to main October 26, 2023 07:59

ivarne changed the base branch from main to v8 October 26, 2023 07:59

ivarne force-pushed the ivarne/multipart-form-data branch from 319ce56 to 7fce5ab Compare October 26, 2023 09:34

github-advanced-security bot found potential problems Oct 26, 2023

View reviewed changes

ivarne force-pushed the ivarne/multipart-form-data branch from 7fce5ab to 03b4317 Compare October 26, 2023 09:48

ivarne mentioned this pull request Nov 6, 2023

Update app-lib-dotnet to dotnet8 with c# 12 #340

Merged

5 tasks

ivarne force-pushed the ivarne/multipart-form-data branch from 03b4317 to cf256d0 Compare November 13, 2023 10:26

tjololo reviewed Nov 15, 2023

View reviewed changes

ivarne commented Nov 15, 2023

View reviewed changes

ivarne force-pushed the ivarne/multipart-form-data branch from 8869639 to b1b1357 Compare November 15, 2023 11:57

tjololo approved these changes Nov 15, 2023

View reviewed changes

src/Altinn.App.Core/Helpers/Serialization/ModelDeserializer.cs Outdated Show resolved Hide resolved

ivarne mentioned this pull request Nov 23, 2023

Add all test data from localtest and add a few missing mocks #362

Merged

5 tasks

ivarne and others added 8 commits November 27, 2023 13:34

Let ModelDeserializer return a Result type instead of nullable

236f553

Remove NullDataProcessor and let user registrer multiple processors

d005192

Update IDataProcessor interface and support multipart data write

9a87e56

Update v7tov8 script for new IDataProcessor interface

c4e456d

Also ignore CS1998 in apps (warning when not awaiting anything in async hooks)

Code review updates

9c2ca7d

Update src/Altinn.App.Core/Helpers/Serialization/ModelDeserializer.cs

6d66fd3

Co-authored-by: Vemund Gaukstad <[email protected]>

First draft of tests for controllers

5f43aac

Finish writing tests

edd91bd

ivarne force-pushed the ivarne/multipart-form-data branch from 1d9c021 to edd91bd Compare November 27, 2023 20:48

ivarne and others added 3 commits November 28, 2023 13:43

Set partyId in token to null

f4f3590

Fix bad case for roles folder in testsetup

a52e601

Add partyId correctly in test tokens

a8ef92e

Also add a Dockerfile for running tests on linux

tjololo added the feature Label Pull requests with new features. Used when generation releasenotes label Nov 29, 2023

ivarne added 4 commits November 30, 2023 08:54

Merge remote-tracking branch 'origin/v8' into ivarne/multipart-form-data

5820855

Use proper parsing library to decode multipart/form-data requests

a2cb36f

Continue returning changed only changed values from data put.

d5e3736

Fix so that it compiles (still failing tests)

6d663fb

github-advanced-security bot found potential problems Nov 30, 2023

View reviewed changes

src/Altinn.App.Api/Controllers/DataController.cs Fixed Show fixed Hide fixed

ivarne added 2 commits December 2, 2023 21:27

Fix tests by disabeling redirects in applciationfactory client

d29d4cc

For some reason backend returns 303 See Other when datamodel updates are availible. This is a redirect code and crashed the tests as no Location header was set.

Add more tests

fa85643

github-advanced-security bot found potential problems Dec 4, 2023

View reviewed changes

test/Altinn.App.Api.Tests/Controllers/InstancesController_PostNewInstance.cs Fixed Show fixed Hide fixed

More tests

d22dfac

tjololo approved these changes Dec 5, 2023

View reviewed changes

src/Altinn.App.Api/Controllers/DataController.cs Outdated Show resolved Hide resolved

ivarne and others added 6 commits December 6, 2023 13:47

Update src/Altinn.App.Api/Controllers/DataController.cs

66ad588

Co-authored-by: Vemund Gaukstad <[email protected]>

Fix code smells

df2d7b1

Fix tests

28e66fa

Tests OK now?

fa06880

Merge remote-tracking branch 'origin/v8' into ivarne/multipart-form-data

d2cd711

More tests for model deserializer

3889386

ivarne merged commit db957c0 into v8 Dec 13, 2023
8 of 10 checks passed

ivarne deleted the ivarne/multipart-form-data branch December 13, 2023 11:57

ivarne commented Dec 19, 2023

View reviewed changes

ivarne added a commit that referenced this pull request Dec 20, 2023

Revert Multipart form data #329 on PUT endpoint

c8f7dc7

ivarne mentioned this pull request Dec 20, 2023

New PATCH endpoint #384

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipart form data #329

Multipart form data #329

ivarne commented Oct 24, 2023 •

edited by olemartinorg

Loading

tjololo left a comment

ivarne left a comment

tjololo left a comment

tjololo commented Nov 29, 2023

olemartinorg commented Nov 29, 2023

olemartinorg commented Dec 4, 2023

tjololo left a comment

sonarcloud bot commented Dec 8, 2023

ivarne Dec 19, 2023

Multipart form data #329

Multipart form data #329

Conversation

ivarne commented Oct 24, 2023 • edited by olemartinorg Loading

Related Issue(s)

Verification

Documentation

tjololo left a comment

Choose a reason for hiding this comment

ivarne left a comment

Choose a reason for hiding this comment

tjololo left a comment

Choose a reason for hiding this comment

tjololo commented Nov 29, 2023

olemartinorg commented Nov 29, 2023

olemartinorg commented Dec 4, 2023

tjololo left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 8, 2023

ivarne Dec 19, 2023

Choose a reason for hiding this comment

ivarne commented Oct 24, 2023 •

edited by olemartinorg

Loading