Ivarne/warning fixes #328

ivarne · 2023-10-18T10:41:07Z

A collection of commits that fixes warnings and add tests where behaviour is changed. For review it might be helpful to look at one commit at the time.

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

adamralph/minver#839 MinVerDefaultPreReleasePhase -> MinVerDefaultPreReleaseIdentifiers

The `[FromQuery]` `?language=` parameter was optional without nullable reference types, but now became required causing a `BadRequest` in tests. It was not decorated as nullable, so a quick fix is to supply a default value of "nb". Other potential resolutions would be * Update the interface so that language is declared nullable for IAppOptionsProvider * Change the public app API, so that language becomes a required get parameter.

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

Note that enabeling nullable on controllers makes non nullable parameters required and causes additional BadRequest responses

…ient backed by KeyVault

tjololo

Nice work! Some comments to evaluate

tjololo · 2023-10-19T08:54:45Z

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

-            if (string.IsNullOrWhiteSpace(dataType))
-            {
-                return BadRequest("Element type must be provided.");
-            }
-


This will change the error response to the user as not providing dataType param will lead to BadReuqest from line 128 Element type {dataType} not allowed for instance {instanceOwnerPartyId}/{instanceGuid}.

No, the addition #nullable enable on line 1 will trigger asp net to return BadRequest with a Problem json structure that explains that dataType is a required part of the query string, because it isn't nullable.

Have you tested this? Might be som error in my test setup, but when I tested it locally it didn't return bad request with Problem json when I omitted the dataType param

Hmm... Not here specifically. I got that behaviour (breaking a test) when running tests under net8.0 with <nullable>enabled</nullable> in project.json. Therefore I was very careful with [FromQuery] parameters that was not nullable. Maybe it's only when nullability is enabled globally, maybe only net8.0?

You were right. The difference was that I tested this with OptionsController and it was annotated with [ApiController] which causes automatic BadRequest for non nullable query strings when nullable is enabled. I wrote a test for this "BadRequest" response and added the annotation to make the test work in ab43649.

There is no difference between dovnet 6-8 or file scoped/project scoped nullable

tjololo · 2023-10-19T09:12:00Z

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

                return StatusCode((int)se.StatusCode, se.Message);
            }

            return StatusCode(500, $"{message}");
        }

-        private async Task<ActionResult> CreateBinaryData(Instance instanceBefore, string dataType, string contentType, string filename, Stream fileStream)
+        private async Task<ActionResult> CreateBinaryData(Instance instanceBefore, string dataType, string contentType, string? filename, Stream fileStream)


Checked the call chain for this method in regards to filename beeing null. Seems to me that DataRestrictionValidation.CompliesWithDataRestrictions that is called from the Post Data method should ensure that the filename is not null, but it's not quite obvious so the compiler might not catch up on it

Nice, I only traced it to being read from a header I think is optional, so I figured it might be null. We should probably store the file with a filename anyway, so a better solution to the nullability problem might be to throw or use Guid.NewGuid()

tjololo · 2023-10-19T09:24:01Z

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

@@ -38,10 +40,10 @@ public OptionsController(IAppOptionsService appOptionsService)
        [HttpGet("{optionsId}")]
        public async Task<IActionResult> Get(
            [FromRoute] string optionsId,
-            [FromQuery] string language,
+            [FromQuery] string? language,


Could we set "nb" as default param value instead. Makes the fact that nb is the default more obvious both in the code and swagger

That was my first attempt, but in C# you can't put a default value in the middle of a parameter list. [FromQuery] Dictionary seems a little bit odd to not have last. I'll try to see if queryParams = null! works.

I see, didn't think of that during my review. Don't but to much effort into fixing my comment!

tjololo · 2023-10-19T09:24:51Z

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

@@ -74,12 +76,12 @@ public OptionsController(IAppOptionsService appOptionsService)
            [FromRoute] int instanceOwnerPartyId,
            [FromRoute] Guid instanceGuid,
            [FromRoute] string optionsId,
-            [FromQuery] string language,
+            [FromQuery] string? language,


Could we set "nb" as default param value instead. Makes the fact that nb is the default more obvious both in the code and swagger

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

ivarne · 2023-10-20T05:56:32Z

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);


log4j: Ooof, we use application insights that shouldn't have any of these issues, but application owners might use other tools. I don't think there is anything we can do preventively here?

Agreed, this change does not make it worse/better and we have alot of log4shell warnings/errors

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);


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

…bad request Also add test

sonarcloud · 2023-10-20T06:04:55Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

23.7% Coverage
0.0% Duplication

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

ivarne · 2023-10-23T07:11:01Z

@tjololo Ser ut som du approvet før jeg gjorde de siste endringene. Vil du godkjenne igjen, eller skal jeg bare merge?

ivarne added 3 commits October 13, 2023 21:06

MinVer update

c092b50

adamralph/minver#839 MinVerDefaultPreReleasePhase -> MinVerDefaultPreReleaseIdentifiers

Don't warn for [Obsolete] api usage or missing XML doc in test projects

903e8d6

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

View reviewed changes

src/Altinn.App.Api/Controllers/ProcessController.cs Dismissed Show resolved Hide resolved

src/Altinn.App.Api/Controllers/ProcessController.cs Dismissed Show resolved Hide resolved

src/Altinn.App.Api/Controllers/ProcessController.cs Dismissed Show resolved Hide resolved

ivarne added 3 commits October 18, 2023 14:15

Fix all the easy nullability warnings in Controllers

d130cab

Note that enabeling nullable on controllers makes non nullable parameters required and causes additional BadRequest responses

Change behaviour of SecretsLocalClient to throw exception like the cl…

de0ef73

…ient backed by KeyVault

Fix nullability warnings in MulipartRequestReader

1c73f06

ivarne force-pushed the ivarne/warning-fixes branch from ffb2d9c to 1c73f06 Compare October 18, 2023 12:16

tjololo self-assigned this Oct 19, 2023

Update sonarColud comment

a3dc24a

tjololo approved these changes Oct 19, 2023

View reviewed changes

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

View reviewed changes

Declare DataController with [ApiController] to automatically trigger …

b7bb5c1

…bad request Also add test

ivarne force-pushed the ivarne/warning-fixes branch from ab43649 to b7bb5c1 Compare October 20, 2023 05:58

ivarne mentioned this pull request Oct 24, 2023

Multipart form data #329

Merged

5 tasks

tjololo approved these changes Nov 13, 2023

View reviewed changes

ivarne merged commit 9c4ed1a into v8 Nov 13, 2023
8 of 10 checks passed

ivarne deleted the ivarne/warning-fixes branch November 13, 2023 09:19

ivarne mentioned this pull request Jan 26, 2024

Make language nullable in option lists and data lists #406

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ivarne/warning fixes #328

Ivarne/warning fixes #328

ivarne commented Oct 18, 2023

tjololo left a comment

tjololo Oct 19, 2023

ivarne Oct 19, 2023

tjololo Oct 19, 2023

ivarne Oct 19, 2023

ivarne Oct 19, 2023 •

edited

Loading

tjololo Oct 19, 2023

ivarne Oct 19, 2023

tjololo Oct 19, 2023

ivarne Oct 19, 2023

tjololo Oct 19, 2023

tjololo Oct 19, 2023

ivarne Oct 20, 2023

tjololo Nov 13, 2023

sonarcloud bot commented Oct 20, 2023

ivarne commented Oct 23, 2023

Ivarne/warning fixes #328

Ivarne/warning fixes #328

Conversation

ivarne commented Oct 18, 2023

Verification

tjololo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarne Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 20, 2023

ivarne commented Oct 23, 2023

ivarne Oct 19, 2023 •

edited

Loading