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

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 all 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
62 changes: 30 additions & 32 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 @@ -30,6 +32,7 @@
/// The data controller handles creation, update, validation and calculation of data elements.
/// </summary>
[AutoValidateAntiforgeryTokenIfAuthCookie]
[ApiController]
[ResponseCache(Duration = 0, Location = ResponseCacheLocation.None, NoStore = true)]
[Route("{org}/{app}/instances/{instanceOwnerPartyId:int}/{instanceGuid:guid}/data")]
public class DataController : ControllerBase
Expand Down Expand Up @@ -113,18 +116,13 @@
[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 +156,7 @@
(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 +173,11 @@
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 +194,7 @@

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

fileStream.Seek(0, SeekOrigin.Begin);
Expand Down Expand Up @@ -258,7 +256,7 @@
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 +317,7 @@
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 @@ -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

This log entry depends on a
user-provided value
Loading
.
This log entry depends on a
user-provided value
Loading
.
Copy link
Member Author

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?

Copy link
Member

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

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.

Check failure

Code scanning / CodeQL

Log entries created from user input High

This log entry depends on a
user-provided value
Loading
.
return BadRequest($"Could not determine if data type {dataType} requires application logic.");
}
else if ((bool)appLogic)
else if (appLogic == true)
{
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 +384,7 @@
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 +419,19 @@
{
_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 +455,7 @@
{
Guid instanceGuid = Guid.Parse(instance.Id.Split("/")[1]);

object appModel;
object? appModel;

string classRef = _appResourcesService.GetClassRefForLogicDataType(dataType);

Expand All @@ -472,7 +468,7 @@
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 +506,7 @@

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 +584,7 @@

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 +598,7 @@
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 +616,7 @@
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 +628,7 @@
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 +648,10 @@
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
Loading
Loading