Skip to content

Commit

Permalink
Ivarne/warning fixes (#328)
Browse files Browse the repository at this point in the history 
* MinVer update

adamralph/minver#839
MinVerDefaultPreReleasePhase -> MinVerDefaultPreReleaseIdentifiers

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

* Enable nullable in options controller

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.

* Fix all the easy nullability warnings in Controllers

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 client backed by KeyVault

* Fix nullability warnings in MulipartRequestReader

* Update sonarColud comment

* Declare DataController with [ApiController] to automatically trigger bad request

Also add test
  • Loading branch information
@ivarne
ivarne authored Nov 13, 2023
1 parent a32abbf commit 9c4ed1a
Show file tree
Hide file tree
Showing 28 changed files with 272 additions and 141 deletions.
2 changes: 1 addition & 1 deletion cli-tools/altinn-app-cli/altinn-app-cli.csproj
View file
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
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
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
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 @@ namespace Altinn.App.Api.Controllers
/// 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 @@ public async Task<ActionResult> Create(
[FromRoute] Guid instanceGuid,
[FromQuery] string dataType)
{
if (string.IsNullOrWhiteSpace(dataType))
{
return BadRequest("Element type must be provided.");
}

/* 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 @@ 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 +173,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 +194,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 +256,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 +317,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 @@ -332,19 +330,19 @@ public async Task<ActionResult> Put(

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
.

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 @@ 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 +419,19 @@ private ActionResult ExceptionResponse(Exception exception, string message)
{
_logger.LogError(exception, message);

Check failure

Code scanning / CodeQL

Log entries created from user input High

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

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)
{
int instanceOwnerPartyId = int.Parse(instanceBefore.Id.Split("/")[0]);
Guid instanceGuid = Guid.Parse(instanceBefore.Id.Split("/")[1]);
Expand All @@ -459,7 +455,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 +468,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 +506,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 +584,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 +598,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 +616,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 +628,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 +648,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
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
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

0 comments on commit 9c4ed1a

Please sign in to comment.