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.

Sign up for GitHub

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

Jump to bottom

Ivarne/warning fixes #328

Merged
merged 8 commits into from
Nov 13, 2023
Merged

Ivarne/warning fixes #328

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c092b50
MinVer update
ivarne Oct 13, 2023
903e8d6
Don't warn for [Obsolete] api usage or missing XML doc in test projects
ivarne Oct 13, 2023
01d7f40
Enable nullable in options controller
ivarne Oct 16, 2023
d130cab
Fix all the easy nullability warnings in Controllers
ivarne Oct 17, 2023
de0ef73
Change behaviour of SecretsLocalClient to throw exception like the cl…
ivarne Oct 18, 2023
1c73f06
Fix nullability warnings in MulipartRequestReader
ivarne Oct 18, 2023
a3dc24a
Update sonarColud comment
ivarne Oct 19, 2023
b7bb5c1
Declare DataController with [ApiController] to automatically trigger …
ivarne Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli-tools/altinn-app-cli/altinn-app-cli.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
</ItemGroup>
<PropertyGroup>
<RepoRoot>$([System.IO.Directory]::GetParent($(MSBuildThisFileDirectory)).Parent.FullName)</RepoRoot>
<MinVerDefaultPreReleasePhase>preview</MinVerDefaultPreReleasePhase>
<MinVerDefaultPreReleaseIdentifiers>preview.0</MinVerDefaultPreReleaseIdentifiers>
<MinVerTagPrefix>altinn-app-cli</MinVerTagPrefix>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<LangVersion>10.0</LangVersion>
Expand Down
4 changes: 2 additions & 2 deletions src/Altinn.App.Api/Altinn.App.Api.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFrameworks>net6.0</TargetFrameworks>
Expand All @@ -13,7 +13,7 @@
<RepositoryType>git</RepositoryType>
<RepositoryUrl>https://github.com/Altinn/app-lib-dotnet</RepositoryUrl>
<IsPackable>true</IsPackable>
<ImplicitUsings>true</ImplicitUsings>
<ImplicitUsings>enable</ImplicitUsings>

<!-- SonarCloud requires a ProjectGuid to separate projects. -->
<ProjectGuid>{E8F29FE8-6B62-41F1-A08C-2A318DD08BB4}</ProjectGuid>
Expand Down
4 changes: 3 additions & 1 deletion src/Altinn.App.Api/Controllers/AuthorizationController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#nullable enable

using Altinn.App.Core.Configuration;
using Altinn.App.Core.Helpers;
using Altinn.App.Core.Internal.Auth;
Expand Down Expand Up @@ -70,7 +72,7 @@ public async Task<ActionResult> GetCurrentParty(bool returnPartyObject = false)
}
}

string cookieValue = Request.Cookies[_settings.GetAltinnPartyCookieName];
string? cookieValue = Request.Cookies[_settings.GetAltinnPartyCookieName];
if (!int.TryParse(cookieValue, out int partyIdFromCookie))
{
partyIdFromCookie = 0;
Expand Down
57 changes: 27 additions & 30 deletions src/Altinn.App.Api/Controllers/DataController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#nullable enable

using System.Net;
using System.Security.Claims;
using Altinn.App.Api.Helpers.RequestHandling;
Expand Down Expand Up @@ -113,18 +115,13 @@ public async Task<ActionResult> Create(
[FromRoute] Guid instanceGuid,
[FromQuery] string dataType)
{
if (string.IsNullOrWhiteSpace(dataType))
{
return BadRequest("Element type must be provided.");
}

Comment on lines -116 to -120
Copy link
Member

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}.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member Author

@ivarne ivarne Oct 19, 2023 •
edited
Loading

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

/* The Body of the request is read much later when it has been made sure it is worth it. */

try
{
Application application = await _appMetadata.GetApplicationMetadata();
DataType dataTypeFromMetadata = application.DataTypes.FirstOrDefault(e => e.Id.Equals(dataType, StringComparison.InvariantCultureIgnoreCase));

DataType? dataTypeFromMetadata = application.DataTypes.FirstOrDefault(e => e.Id.Equals(dataType, StringComparison.InvariantCultureIgnoreCase));

if (dataTypeFromMetadata == null)
{
Expand Down Expand Up @@ -158,7 +155,7 @@ public async Task<ActionResult> Create(
(bool validationRestrictionSuccess, List<ValidationIssue> errors) = DataRestrictionValidation.CompliesWithDataRestrictions(Request, dataTypeFromMetadata);
if (!validationRestrictionSuccess)
{
return new BadRequestObjectResult(await GetErrorDetails(errors));
return BadRequest(await GetErrorDetails(errors));
}

StreamContent streamContent = Request.CreateContentStream();
Expand All @@ -175,11 +172,11 @@ public async Task<ActionResult> Create(
Description = errorMessage
};
_logger.LogError(errorMessage);
return new BadRequestObjectResult(await GetErrorDetails(new List<ValidationIssue> { error }));
return BadRequest(await GetErrorDetails(new List<ValidationIssue> { error }));
}

bool parseSuccess = Request.Headers.TryGetValue("Content-Disposition", out StringValues headerValues);
string filename = parseSuccess ? DataRestrictionValidation.GetFileNameFromHeader(headerValues) : string.Empty;
string? filename = parseSuccess ? DataRestrictionValidation.GetFileNameFromHeader(headerValues) : null;

IEnumerable<FileAnalysisResult> fileAnalysisResults = new List<FileAnalysisResult>();
if (FileAnalysisEnabledForDataType(dataTypeFromMetadata))
Expand All @@ -196,7 +193,7 @@ public async Task<ActionResult> Create(

if (!fileValidationSuccess)
{
return new BadRequestObjectResult(await GetErrorDetails(validationIssues));
return BadRequest(await GetErrorDetails(validationIssues));
}

fileStream.Seek(0, SeekOrigin.Begin);
Expand Down Expand Up @@ -258,7 +255,7 @@ public async Task<ActionResult> Get(
return NotFound($"Did not find instance {instance}");
}

DataElement dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));
DataElement? dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));

if (dataElement == null)
{
Expand Down Expand Up @@ -319,7 +316,7 @@ public async Task<ActionResult> Put(
return Conflict($"Cannot update data element of archived or deleted instance {instanceOwnerPartyId}/{instanceGuid}");
}

DataElement dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));
DataElement? dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));

if (dataElement == null)
{
Expand All @@ -340,11 +337,11 @@ public async Task<ActionResult> Put(
return await PutFormData(org, app, instance, dataGuid, dataType);
}

DataType dataTypeFromMetadata = (await _appMetadata.GetApplicationMetadata()).DataTypes.FirstOrDefault(e => e.Id.Equals(dataType, StringComparison.InvariantCultureIgnoreCase));
DataType? dataTypeFromMetadata = (await _appMetadata.GetApplicationMetadata()).DataTypes.FirstOrDefault(e => e.Id.Equals(dataType, StringComparison.InvariantCultureIgnoreCase));
(bool validationRestrictionSuccess, List<ValidationIssue> errors) = DataRestrictionValidation.CompliesWithDataRestrictions(Request, dataTypeFromMetadata);
if (!validationRestrictionSuccess)
{
return new BadRequestObjectResult(await GetErrorDetails(errors));
return BadRequest(await GetErrorDetails(errors));
}

return await PutBinaryData(instanceOwnerPartyId, instanceGuid, dataGuid);
Expand Down Expand Up @@ -386,7 +383,7 @@ public async Task<ActionResult> Delete(
return Conflict($"Cannot delete data element of archived or deleted instance {instanceOwnerPartyId}/{instanceGuid}");
}

DataElement dataElement = instance.Data.Find(m => m.Id.Equals(dataGuid.ToString()));
DataElement? dataElement = instance.Data.Find(m => m.Id.Equals(dataGuid.ToString()));

if (dataElement == null)
{
Expand Down Expand Up @@ -421,21 +418,19 @@ private ActionResult ExceptionResponse(Exception exception, string message)
{
_logger.LogError(exception, message);

if (exception is PlatformHttpException)
if (exception is PlatformHttpException phe)
{
PlatformHttpException phe = exception as PlatformHttpException;
return StatusCode((int)phe.Response.StatusCode, phe.Message);
}
else if (exception is ServiceException)
else if (exception is ServiceException se)
{
ServiceException se = exception as ServiceException;
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)
Copy link
Member

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

Copy link
Member Author

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

{
int instanceOwnerPartyId = int.Parse(instanceBefore.Id.Split("/")[0]);
Guid instanceGuid = Guid.Parse(instanceBefore.Id.Split("/")[1]);
Expand All @@ -459,7 +454,7 @@ private async Task<ActionResult> CreateAppModelData(
{
Guid instanceGuid = Guid.Parse(instance.Id.Split("/")[1]);

object appModel;
object? appModel;

string classRef = _appResourcesService.GetClassRefForLogicDataType(dataType);

Expand All @@ -472,7 +467,7 @@ private async Task<ActionResult> CreateAppModelData(
ModelDeserializer deserializer = new ModelDeserializer(_logger, _appModel.GetModelType(classRef));
appModel = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);

if (!string.IsNullOrEmpty(deserializer.Error))
if (!string.IsNullOrEmpty(deserializer.Error) || appModel is null)
{
return BadRequest(deserializer.Error);
}
Expand Down Expand Up @@ -510,7 +505,7 @@ private async Task<ActionResult> GetBinaryData(

if (dataStream != null)
{
string userOrgClaim = User.GetOrg();
string? userOrgClaim = User.GetOrg();
if (userOrgClaim == null || !org.Equals(userOrgClaim, StringComparison.InvariantCultureIgnoreCase))
{
await _instanceClient.UpdateReadStatus(instanceOwnerPartyId, instanceGuid, "read");
Expand Down Expand Up @@ -588,7 +583,7 @@ private async Task<ActionResult> GetFormData(

await _dataProcessor.ProcessDataRead(instance, dataGuid, appModel);

string userOrgClaim = User.GetOrg();
string? userOrgClaim = User.GetOrg();
if (userOrgClaim == null || !org.Equals(userOrgClaim, StringComparison.InvariantCultureIgnoreCase))
{
await _instanceClient.UpdateReadStatus(instanceOwnerId, instanceGuid, "read");
Expand All @@ -602,7 +597,7 @@ private async Task<ActionResult> PutBinaryData(int instanceOwnerPartyId, Guid in
if (Request.Headers.TryGetValue("Content-Disposition", out StringValues headerValues))
{
var contentDispositionHeader = ContentDispositionHeaderValue.Parse(headerValues.ToString());
_logger.LogInformation("Content-Disposition: {ContentDisposition}", headerValues);
_logger.LogInformation("Content-Disposition: {ContentDisposition}", headerValues.ToString());
DataElement dataElement = await _dataClient.UpdateBinaryData(new InstanceIdentifier(instanceOwnerPartyId, instanceGuid), Request.ContentType, contentDispositionHeader.FileName.ToString(), dataGuid, Request.Body);
SelfLinkHelper.SetDataAppSelfLinks(instanceOwnerPartyId, instanceGuid, dataElement, Request);

Expand All @@ -620,7 +615,7 @@ private async Task<ActionResult> PutFormData(string org, string app, Instance in
Guid instanceGuid = Guid.Parse(instance.Id.Split("/")[1]);

ModelDeserializer deserializer = new ModelDeserializer(_logger, _appModel.GetModelType(classRef));
object serviceModel = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);
object? serviceModel = await deserializer.DeserializeAsync(Request.Body, Request.ContentType);

if (!string.IsNullOrEmpty(deserializer.Error))
{
Expand All @@ -632,7 +627,7 @@ private async Task<ActionResult> PutFormData(string org, string app, Instance in
return BadRequest("No data found in content");
}

Dictionary<string, object> changedFields = await JsonHelper.ProcessDataWriteWithDiff(instance, dataGuid, serviceModel, _dataProcessor, _logger);
Dictionary<string, object?>? changedFields = await JsonHelper.ProcessDataWriteWithDiff(instance, dataGuid, serviceModel, _dataProcessor, _logger);

await UpdatePresentationTextsOnInstance(instance, dataType, serviceModel);
await UpdateDataValuesOnInstance(instance, dataType, serviceModel);
Expand All @@ -652,8 +647,10 @@ private async Task<ActionResult> PutFormData(string org, string app, Instance in
string dataUrl = updatedDataElement.SelfLinks.Apps;
if (changedFields is not null)
{
CalculationResult calculationResult = new CalculationResult(updatedDataElement);
calculationResult.ChangedFields = changedFields;
CalculationResult calculationResult = new(updatedDataElement)
{
ChangedFields = changedFields
};
return StatusCode((int)HttpStatusCode.SeeOther, calculationResult);
}

Expand Down
7 changes: 4 additions & 3 deletions src/Altinn.App.Api/Controllers/DataTagsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#nullable enable
using System.Net.Mime;
using System.Text.RegularExpressions;

Expand Down Expand Up @@ -65,7 +66,7 @@ public async Task<ActionResult<TagsList>> Get(
return NotFound($"Unable to find instance based on the given parameters.");
}

DataElement dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));
DataElement? dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));

if (dataElement == null)
{
Expand Down Expand Up @@ -112,7 +113,7 @@ public async Task<ActionResult<TagsList>> Add(
return NotFound("Unable to find instance based on the given parameters.");
}

DataElement dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));
DataElement? dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));

if (dataElement == null)
{
Expand Down Expand Up @@ -161,7 +162,7 @@ public async Task<ActionResult> Delete(
return NotFound("Unable to find instance based on the given parameters.");
}

DataElement dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));
DataElement? dataElement = instance.Data.FirstOrDefault(m => m.Id.Equals(dataGuid.ToString()));

if (dataElement == null)
{
Expand Down
16 changes: 8 additions & 8 deletions src/Altinn.App.Api/Controllers/InstancesController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,14 @@ public async Task<ActionResult<Instance>> Post(
// create minimum instance template
instanceTemplate = new Instance
{
InstanceOwner = new InstanceOwner { PartyId = instanceOwnerPartyId.Value.ToString() }
InstanceOwner = new InstanceOwner { PartyId = instanceOwnerPartyId!.Value.ToString() }
};
}

ApplicationMetadata application = await _appMetadata.GetApplicationMetadata();

RequestPartValidator requestValidator = new RequestPartValidator(application);
string multipartError = requestValidator.ValidateParts(parsedRequest.Parts);
string? multipartError = requestValidator.ValidateParts(parsedRequest.Parts);

if (!string.IsNullOrEmpty(multipartError))
{
Expand Down Expand Up @@ -469,7 +469,7 @@ public async Task<ActionResult<Instance>> PostSimplified(

instance = await _instanceClient.CreateInstance(org, app, instanceTemplate);

if (copySourceInstance)
if (copySourceInstance && source is not null)
{
await CopyDataFromSourceInstance(application, instance, source);
}
Expand Down Expand Up @@ -958,7 +958,7 @@ private async Task StorePrefillParts(Instance instance, ApplicationMetadata appI
DataElement dataElement;
if (dataType?.AppLogic?.ClassRef != null)
{
_logger.LogInformation($"Storing part {part.Name}");
_logger.LogInformation("Storing part {partName}", part.Name);

Type type;
try
Expand All @@ -973,12 +973,12 @@ private async Task StorePrefillParts(Instance instance, ApplicationMetadata appI
ModelDeserializer deserializer = new ModelDeserializer(_logger, type);
object? data = await deserializer.DeserializeAsync(part.Stream, part.ContentType);

if (!string.IsNullOrEmpty(deserializer.Error))
if (!string.IsNullOrEmpty(deserializer.Error) || data is null)
{
throw new InvalidOperationException(deserializer.Error);
}

await _prefillService.PrefillDataModel(instance.InstanceOwner.PartyId, part.Name, data);
await _prefillService.PrefillDataModel(instance.InstanceOwner.PartyId, part.Name!, data);

await _instantiationProcessor.DataCreation(instance, data, null);

Expand All @@ -989,11 +989,11 @@ private async Task StorePrefillParts(Instance instance, ApplicationMetadata appI
org,
app,
instanceOwnerIdAsInt,
part.Name);
part.Name!);
}
else
{
dataElement = await _dataClient.InsertBinaryData(instance.Id, part.Name, part.ContentType, part.FileName, part.Stream);
dataElement = await _dataClient.InsertBinaryData(instance.Id, part.Name!, part.ContentType, part.FileName, part.Stream);
}

if (dataElement == null)
Expand Down
10 changes: 6 additions & 4 deletions src/Altinn.App.Api/Controllers/OptionsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
#nullable enable

using System;
using System.Collections.Generic;
using System.Threading.Tasks;
Expand Down Expand Up @@ -38,10 +40,10 @@ public OptionsController(IAppOptionsService appOptionsService)
[HttpGet("{optionsId}")]
public async Task<IActionResult> Get(
[FromRoute] string optionsId,
[FromQuery] string language,
[FromQuery] string? language,
Copy link
Member

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

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 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.

Copy link
Member

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!

[FromQuery] Dictionary<string, string> queryParams)
{
AppOptions appOptions = await _appOptionsService.GetOptionsAsync(optionsId, language, queryParams);
AppOptions appOptions = await _appOptionsService.GetOptionsAsync(optionsId, language ?? "nb", queryParams);
if (appOptions.Options == null)
{
return NotFound();
Expand Down Expand Up @@ -74,12 +76,12 @@ public async Task<IActionResult> Get(
[FromRoute] int instanceOwnerPartyId,
[FromRoute] Guid instanceGuid,
[FromRoute] string optionsId,
[FromQuery] string language,
[FromQuery] string? language,
Copy link
Member

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

[FromQuery] Dictionary<string, string> queryParams)
{
var instanceIdentifier = new InstanceIdentifier(instanceOwnerPartyId, instanceGuid);

AppOptions appOptions = await _appOptionsService.GetOptionsAsync(instanceIdentifier, optionsId, language, queryParams);
AppOptions appOptions = await _appOptionsService.GetOptionsAsync(instanceIdentifier, optionsId, language ?? "nb", queryParams);

// Only return NotFound if we can't find an options provider.
// If we find the options provider, but it doesnt' have values, return empty list.
Expand Down
Loading
Loading