-
Notifications
You must be signed in to change notification settings - Fork 10
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
Ivarne/warning fixes #328
Ivarne/warning fixes #328
Conversation
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.
Note that enabeling nullable on controllers makes non nullable parameters required and causes additional BadRequest responses
…ient backed by KeyVault
ffb2d9c
to
1c73f06
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Some comments to evaluate
if (string.IsNullOrWhiteSpace(dataType)) | ||
{ | ||
return BadRequest("Element type must be provided."); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
@@ -38,10 +40,10 @@ public OptionsController(IAppOptionsService appOptionsService) | |||
[HttpGet("{optionsId}")] | |||
public async Task<IActionResult> Get( | |||
[FromRoute] string optionsId, | |||
[FromQuery] string language, | |||
[FromQuery] string? language, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, didn't think of that during my review. Don't but to much effort into fixing my comment!
@@ -74,12 +76,12 @@ public OptionsController(IAppOptionsService appOptionsService) | |||
[FromRoute] int instanceOwnerPartyId, | |||
[FromRoute] Guid instanceGuid, | |||
[FromRoute] string optionsId, | |||
[FromQuery] string language, | |||
[FromQuery] string? language, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
This log entry depends on a
user-provided value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this change does not make it worse/better and we have alot of log4shell warnings/errors
@@ -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); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
@@ -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); |
Check failure
Code scanning / CodeQL
Log entries created from user input High
user-provided value
…bad request Also add test
ab43649
to
b7bb5c1
Compare
SonarCloud Quality Gate failed. 0 Bugs 23.7% Coverage Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
@tjololo Ser ut som du approvet før jeg gjorde de siste endringene. Vil du godkjenne igjen, eller skal jeg bare merge? |
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