From 815a82d12d811111d009479c1c96904a405bdfe5 Mon Sep 17 00:00:00 2001 From: Adam Hopkins <127156771+simpat-adam@users.noreply.github.com> Date: Fri, 25 Oct 2024 10:26:20 -0500 Subject: [PATCH] [DMS-353] Validate method allowed and build script fixes (#320) --- .github/workflows/on-dms-pullrequest.yml | 3 - build-dms.ps1 | 35 +++-- eng/docker-compose/setup-connectors.ps1 | 118 ++++++++-------- eng/docker-compose/start-local-dms.ps1 | 26 ++-- .../Middleware/ParsePathMiddlewareTests.cs | 128 +++++++++++++++++- .../Middleware/ParsePathMiddleware.cs | 35 +++++ .../Response/FailureResponse.cs | 16 ++- .../UpdateDescriptorsValidation.feature | 2 +- .../Features/General/URLValidation.feature | 24 ++-- 9 files changed, 288 insertions(+), 99 deletions(-) diff --git a/.github/workflows/on-dms-pullrequest.yml b/.github/workflows/on-dms-pullrequest.yml index 888d8012b..e45eb2310 100644 --- a/.github/workflows/on-dms-pullrequest.yml +++ b/.github/workflows/on-dms-pullrequest.yml @@ -179,9 +179,6 @@ jobs: - name: Checkout the Repo uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1 - - name: Build - run: ./build-dms.ps1 Build -Configuration Debug - - name: Run OpenSearch End to End Tests if: success() run: ./build-dms.ps1 E2ETest -EnableOpenSearch diff --git a/build-dms.ps1 b/build-dms.ps1 index 7c9c8c0a0..5d1c71006 100644 --- a/build-dms.ps1 +++ b/build-dms.ps1 @@ -242,17 +242,21 @@ function RunE2E { } function E2ETests { + Invoke-Step { DockerBuild } + if ($EnableOpenSearch) { - try { - Push-Location eng/docker-compose/ - ./start-local-dms.ps1 "./.env.e2e" - } - finally { - Pop-Location + Invoke-Execute { + try { + Push-Location eng/docker-compose/ + ./start-local-dms.ps1 -EnvironmentFile "./.env.e2e" + } + finally { + Pop-Location + } } } else { - Invoke-Step { DockerBuild } + Invoke-Step { DockerRun } } Invoke-Step { RunE2E } } @@ -273,7 +277,13 @@ function RunNuGetPack { # NU5100 is the warning about DLLs outside of a "lib" folder. We're # deliberately using that pattern, therefore we bypass the # warning. - dotnet pack $ProjectPath --no-build --no-restore --output $PSScriptRoot -p:NuspecFile=$nuspecPath -p:NuspecProperties="version=$PackageVersion;year=$copyrightYear" /p:NoWarn=NU5100 + dotnet pack $ProjectPath ` + --no-build ` + --no-restore ` + --output $PSScriptRoot ` + -p:NuspecFile=$nuspecPath ` + -p:NuspecProperties="version=$PackageVersion;year=$copyrightYear" ` + /p:NoWarn=NU5100 } function BuildPackage { @@ -314,10 +324,13 @@ function Invoke-TestExecution { IgnoreCase = $true)] # File search filter [string] - $Filter + $Filter, + + [switch] + $EnableOpenSearch ) switch ($Filter) { - E2ETests { Invoke-Step { E2ETests } } + E2ETests { Invoke-Step { E2ETests -EnableOpenSearch:$EnableOpenSearch } } UnitTests { Invoke-Step { UnitTests } } IntegrationTests { Invoke-Step { IntegrationTests } } Default { "Unknow Test Type" } @@ -398,7 +411,7 @@ Invoke-Main { Invoke-Publish } UnitTest { Invoke-TestExecution UnitTests } - E2ETest { Invoke-TestExecution E2ETests } + E2ETest { Invoke-TestExecution E2ETests -EnableOpenSearch:$EnableOpenSearch } IntegrationTest { Invoke-TestExecution IntegrationTests } Coverage { Invoke-Coverage } Package { Invoke-BuildPackage } diff --git a/eng/docker-compose/setup-connectors.ps1 b/eng/docker-compose/setup-connectors.ps1 index e1cad99f4..dc034672f 100644 --- a/eng/docker-compose/setup-connectors.ps1 +++ b/eng/docker-compose/setup-connectors.ps1 @@ -6,8 +6,7 @@ param ( $EnvironmentFile = "./.env" ) -function IsReady([string] $Url) -{ +function IsReady([string] $Url) { $maxAttempts = 6 $attempt = 0 $waitTime = 5 @@ -41,70 +40,73 @@ catch { Write-Error "Please provide valid .env file." } -$sourcePort=$envFile["CONNECT_SOURCE_PORT"] -$sinkPort=$envFile["CONNECT_SINK_PORT"] +$sourcePort = $envFile["CONNECT_SOURCE_PORT"] +$sinkPort = $envFile["CONNECT_SINK_PORT"] $sourceBase = "http://localhost:$sourcePort/connectors" $sinkBase = "http://localhost:$sinkPort/connectors" $sourceUrl = "$sourceBase/postgresql-source" $sinkUrl = "$sinkBase/opensearch-sink" - # Source connector - if(IsReady($sourceBase)) - { - try { - $sourceResponse = Invoke-RestMethod -Uri $sourceUrl -Method Get - if($sourceResponse) - { - Write-Output "Deleting existing source connector configuration." - Invoke-RestMethod -Method Delete -uri $sourceUrl - } - } - catch { +# Source connector +if (IsReady($sourceBase)) { + try { + $sourceResponse = Invoke-RestMethod -Uri $sourceUrl -Method Get -SkipHttpErrorCheck + + # only true if the response was 200 + if ($null -ne $sourceResponse.name) { + Write-Output "Deleting existing source connector configuration." + Invoke-RestMethod -Method Delete -uri $sourceUrl + } + } + catch { + Write-Output $_.Exception.Message + } + + try { + $sourceBody = Get-Content "./postgresql_connector.json" + $sourceBody = $sourceBody.Replace("abcdefgh1!", $envFile["POSTGRES_PASSWORD"]) + + Write-Output "Installing source connector configuration" + Invoke-RestMethod -Method Post -uri $sourceBase -ContentType "application/json" -Body $sourceBody + } + catch { Write-Output $_.Exception.Message - } + } + Start-Sleep 2 + Invoke-RestMethod -Method Get -uri $sourceUrl -SkipHttpErrorCheck +} +else { + Write-Output "Service at $sourceBase not available." +} - try { - $sourceBody = Get-Content "./postgresql_connector.json" - $sourceBody = $sourceBody.Replace("abcdefgh1!", $envFile["POSTGRES_PASSWORD"]) - Invoke-RestMethod -Method Post -uri $sourceBase -ContentType "application/json" -Body $sourceBody - } - catch { - Write-Output $_.Exception.Message - } - Start-Sleep 2 - Invoke-RestMethod -Method Get -uri $sourceUrl - } - else { - Write-Output "Service at $sourceBase not available." - } +# Sink connector +if (IsReady($sinkBase)) { + try { + $sinkResponse = Invoke-RestMethod -Uri $sinkUrl -Method Get -SkipHttpErrorCheck - # Sink connector - if(IsReady($sinkBase)) - { - try { - $sinkResponse = Invoke-RestMethod -Uri $sinkUrl -Method Get - if($sinkResponse) - { - Write-Output "Deleting existing sink connector configuration." - Invoke-RestMethod -Method Delete -uri $sinkUrl - } - } - catch { + if ($null -ne $sinkResponse.name) { + Write-Output "Deleting existing sink connector configuration." + Invoke-RestMethod -Method Delete -uri $sinkUrl + } + } + catch { Write-Output $_.Exception.Message - } + } + + try { + $sinkBody = Get-Content "./opensearch_connector.json" + $sinkBody = $sinkBody.Replace("abcdefgh1!", $envFile["OPENSEARCH_ADMIN_PASSWORD"]) - try { - $sinkBody = Get-Content "./opensearch_connector.json" - $sinkBody = $sinkBody.Replace("abcdefgh1!", $envFile["OPENSEARCH_ADMIN_PASSWORD"]) - Invoke-RestMethod -Method Post -uri $sinkBase -ContentType "application/json" -Body $sinkBody - } - catch { - Write-Output $_.Exception.Message - } - Start-Sleep 2 - Invoke-RestMethod -Method Get -uri $sinkUrl - } - else { - Write-Output "Service at $sinkBase not available." - } + Write-Output "Installing sink connector configuration" + Invoke-RestMethod -Method Post -uri $sinkBase -ContentType "application/json" -Body $sinkBody + } + catch { + Write-Output $_.Exception.Message + } + Start-Sleep 2 + Invoke-RestMethod -Method Get -uri $sinkUrl -SkipHttpErrorCheck +} +else { + Write-Output "Service at $sinkBase not available." +} diff --git a/eng/docker-compose/start-local-dms.ps1 b/eng/docker-compose/start-local-dms.ps1 index 1c4de7e7d..722fbd7b7 100644 --- a/eng/docker-compose/start-local-dms.ps1 +++ b/eng/docker-compose/start-local-dms.ps1 @@ -39,13 +39,11 @@ $files = @( "local-dms.yml" ) -if($EnforceAuthorization) -{ +if ($EnforceAuthorization) { $files += @("-f", "keycloak.yml") } -if($EnableOpenSearchUI) -{ +if ($EnableOpenSearchUI) { $files += @("-f", "kafka-opensearch-ui.yml") } @@ -60,18 +58,24 @@ if ($d) { } } else { - $upArgs = @() - if ($r) { $upArgs += "--build" } + $upArgs = @( + "--detach" + ) + if ($r) { $upArgs += @("--build") } Write-Output "Starting locally-built DMS" - if($EnforceAuthorization) - { - $env:IDENTITY_ENFORCE_AUTHORIZATION=$true + if ($EnforceAuthorization) { + $env:IDENTITY_ENFORCE_AUTHORIZATION = $true } else { - $env:IDENTITY_ENFORCE_AUTHORIZATION=$false + $env:IDENTITY_ENFORCE_AUTHORIZATION = $false + } + + docker compose $files --env-file $EnvironmentFile up $upArgs + + if ($LASTEXITCODE -ne 0) { + throw "Unable to start local Docker environment, with exit code $LASTEXITCODE." } - docker compose $files --env-file $EnvironmentFile up -d $upArg Start-Sleep 20 ./setup-connectors.ps1 $EnvironmentFile diff --git a/src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Middleware/ParsePathMiddlewareTests.cs b/src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Middleware/ParsePathMiddlewareTests.cs index b34736d7e..1c2148d8c 100644 --- a/src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Middleware/ParsePathMiddlewareTests.cs +++ b/src/dms/core/EdFi.DataManagementService.Core.Tests.Unit/Middleware/ParsePathMiddlewareTests.cs @@ -125,7 +125,7 @@ public async Task Setup() QueryParameters: [], TraceId: new TraceId("") ); - _context = new(frontendRequest, RequestMethod.POST); + _context = new(frontendRequest, RequestMethod.PUT); await Middleware().Execute(_context, NullNext); } @@ -187,4 +187,130 @@ public void It_returns_invalid_Id_message() .Contain("\"validationErrors\":{\"$.id\":[\"The value 'invalidId' is not valid.\"]}"); } } + + [TestFixture] + public class Given_A_Post_With_ResourceId : ParsePathMiddlewareTests + { + private PipelineContext _context = No.PipelineContext(); + + [SetUp] + public async Task Setup() + { + FrontendRequest frontendRequest = + new( + Body: "{}", + Path: $"/ed-fi/endpointName/{Guid.NewGuid()}", + QueryParameters: [], + TraceId: new TraceId("") + ); + _context = new(frontendRequest, RequestMethod.POST); + await Middleware().Execute(_context, NullNext); + } + + [Test] + public void It_has_a_response() + { + _context?.FrontendResponse.Should().NotBe(No.FrontendResponse); + } + + [Test] + public void It_returns_status_405() + { + _context?.FrontendResponse.StatusCode.Should().Be(405); + } + + [Test] + public void It_returns_method_not_allowed_message() + { + string response = JsonSerializer.Serialize(_context.FrontendResponse.Body, SerializerOptions); + + response + .Should() + .Contain("Method Not Allowed"); + } + } + + [TestFixture] + public class Given_A_Put_With_Missing_ResourceId : ParsePathMiddlewareTests + { + private PipelineContext _context = No.PipelineContext(); + + [SetUp] + public async Task Setup() + { + FrontendRequest frontendRequest = + new( + Body: "{}", + Path: "/ed-fi/endpointName/", + QueryParameters: [], + TraceId: new TraceId("") + ); + _context = new(frontendRequest, RequestMethod.PUT); + await Middleware().Execute(_context, NullNext); + } + + [Test] + public void It_has_a_response() + { + _context?.FrontendResponse.Should().NotBe(No.FrontendResponse); + } + + [Test] + public void It_returns_status_405() + { + _context?.FrontendResponse.StatusCode.Should().Be(405); + } + + [Test] + public void It_returns_method_not_allowed_message() + { + string response = JsonSerializer.Serialize(_context.FrontendResponse.Body, SerializerOptions); + + response + .Should() + .Contain("Method Not Allowed"); + } + } + + [TestFixture] + public class Given_A_Delete_With_Missing_ResourceId : ParsePathMiddlewareTests + { + private PipelineContext _context = No.PipelineContext(); + + [SetUp] + public async Task Setup() + { + FrontendRequest frontendRequest = + new( + Body: "{}", + Path: "/ed-fi/endpointName/", + QueryParameters: [], + TraceId: new TraceId("") + ); + _context = new(frontendRequest, RequestMethod.DELETE); + await Middleware().Execute(_context, NullNext); + } + + [Test] + public void It_has_a_response() + { + _context?.FrontendResponse.Should().NotBe(No.FrontendResponse); + } + + [Test] + public void It_returns_status_405() + { + _context?.FrontendResponse.StatusCode.Should().Be(405); + } + + [Test] + public void It_returns_method_not_allowed_message() + { + string response = JsonSerializer.Serialize(_context.FrontendResponse.Body, SerializerOptions); + + response + .Should() + .Contain("Method Not Allowed"); + } + } } diff --git a/src/dms/core/EdFi.DataManagementService.Core/Middleware/ParsePathMiddleware.cs b/src/dms/core/EdFi.DataManagementService.Core/Middleware/ParsePathMiddleware.cs index 1a230b041..825f77a4f 100644 --- a/src/dms/core/EdFi.DataManagementService.Core/Middleware/ParsePathMiddleware.cs +++ b/src/dms/core/EdFi.DataManagementService.Core/Middleware/ParsePathMiddleware.cs @@ -97,6 +97,26 @@ public async Task Execute(PipelineContext context, Func next) return; } + // Verify method allowed with/without documentUuid + switch (context.Method) + { + case RequestMethod.DELETE when pathInfo.DocumentUuid == null: + { + NotAllowed(["Resource collections cannot be deleted. To delete a specific item, use DELETE and include the 'id' in the route."]); + return; + } + case RequestMethod.PUT when pathInfo.DocumentUuid == null: + { + NotAllowed(["Resource collections cannot be replaced. To 'upsert' an item in the collection, use POST. To update a specific item, use PUT and include the 'id' in the route."]); + return; + } + case RequestMethod.POST when pathInfo.DocumentUuid != null: + { + NotAllowed(["Resource items can only be updated using PUT. To 'upsert' an item in the resource collection using POST, remove the 'id' from the route."]); + return; + } + } + DocumentUuid documentUuid = pathInfo.DocumentUuid == null ? No.DocumentUuid : new(new(pathInfo.DocumentUuid)); @@ -107,5 +127,20 @@ public async Task Execute(PipelineContext context, Func next) ); await next(); + return; + + void NotAllowed(string[] errors) + { + _logger.LogDebug("ParsePathMiddleware: Missing document UUID on request method {Method}", context.Method); + context.FrontendResponse = new FrontendResponse( + StatusCode: 405, + Body: FailureResponse.ForMethodNotAllowed( + errors, + traceId: context.FrontendRequest.TraceId + ), + Headers: [], + ContentType: "application/json; charset=utf-8" + ); + } } } diff --git a/src/dms/core/EdFi.DataManagementService.Core/Response/FailureResponse.cs b/src/dms/core/EdFi.DataManagementService.Core/Response/FailureResponse.cs index 10ecb6dc5..066b47db8 100644 --- a/src/dms/core/EdFi.DataManagementService.Core/Response/FailureResponse.cs +++ b/src/dms/core/EdFi.DataManagementService.Core/Response/FailureResponse.cs @@ -20,6 +20,7 @@ internal static class FailureResponse private static readonly string _dataConflictTypePrefix = $"{_typePrefix}:data-conflict"; private static readonly string _keyChangeNotSupported = $"{_badRequestTypePrefix}:data-validation-failed:key-change-not-supported"; + private static readonly string _methodNotAllowed = $"{_typePrefix}:method-not-allowed"; private static JsonObject CreateBaseJsonObject( string detail, @@ -126,9 +127,9 @@ public static JsonNode ForInvalidReferences(ResourceName[] resourceNames, TraceI ); } - public static JsonNode ForImmutableIdentity(string error, TraceId traceId) => + public static JsonNode ForImmutableIdentity(string detail, TraceId traceId) => CreateBaseJsonObject( - detail: error, + detail: detail, type: _keyChangeNotSupported, title: "Key Change Not Supported", status: 400, @@ -136,4 +137,15 @@ public static JsonNode ForImmutableIdentity(string error, TraceId traceId) => validationErrors: [], errors: [] ); + + public static JsonNode ForMethodNotAllowed(string[] errors, TraceId traceId) => + CreateBaseJsonObject( + detail: "The request construction was invalid.", + type: _methodNotAllowed, + title: "Method Not Allowed", + status: 405, + correlationId: traceId.Value, + validationErrors: [], + errors + ); } diff --git a/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/Descriptors/UpdateDescriptorsValidation.feature b/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/Descriptors/UpdateDescriptorsValidation.feature index c526c2f66..2822af550 100644 --- a/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/Descriptors/UpdateDescriptorsValidation.feature +++ b/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/Descriptors/UpdateDescriptorsValidation.feature @@ -492,7 +492,7 @@ Feature: Update a Descriptor @API-049 Scenario: 17 Ensure clients cannot update a descriptor omitting any of the required values - When a PUT request is made to "/ed-fi/disabilityDescriptors" with + When a PUT request is made to "/ed-fi/disabilityDescriptors/{id}" with """ { "id": "{id}", diff --git a/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/General/URLValidation.feature b/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/General/URLValidation.feature index c1afc78de..8fc814802 100644 --- a/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/General/URLValidation.feature +++ b/src/dms/tests/EdFi.DataManagementService.Tests.E2E/Features/General/URLValidation.feature @@ -209,7 +209,7 @@ Feature: Validation of the structure of the URLs @API-073 Scenario: 07 Ensure clients cannot update a resource when endpoint does not end in plural - When a PUT request is made to "/ed-fi/school/{id}" with + When a PUT request is made to "/ed-fi/school/00000000-0000-4000-a000-000000000000" with """ { "educationOrganizationCategories": [ @@ -248,7 +248,7 @@ Feature: Validation of the structure of the URLs @API-074 Scenario: 08 Ensure clients cannot delete a resource when endpoint does not end in plural - When a DELETE request is made to "/ed-fi/school/{id}" + When a DELETE request is made to "/ed-fi/school/00000000-0000-4000-a000-000000000000" Then it should respond with 404 And the response body is """ @@ -269,10 +269,9 @@ Feature: Validation of the structure of the URLs } """ - ## The resolution of this ticket will solve the execution error: https://edfi.atlassian.net/browse/DMS-353 - @API-075 @ignore + @API-075 Scenario: 09 Ensure clients cannot create a resource adding an ID as a path variable - When a POST request is made to "/ed-fi/schools/0123456789" with + When a POST request is made to "/ed-fi/schools/00000000-0000-4000-a000-000000000000" with """ { "educationOrganizationCategories": [ @@ -298,8 +297,9 @@ Feature: Validation of the structure of the URLs "title": "Method Not Allowed", "status": 405, "correlationId": null, + "validationErrors": {}, "errors": [ - "Resource items can only be updated using PUT. To \\"upsert\\" an item in the resource collection using POST, remove the \\"id\\" from the route." + "Resource items can only be updated using PUT. To 'upsert' an item in the resource collection using POST, remove the 'id' from the route." ] } """ @@ -310,8 +310,7 @@ Feature: Validation of the structure of the URLs } """ - ## The resolution of this ticket will solve the execution error: https://edfi.atlassian.net/browse/DMS-353 - @API-077 @ignore + @API-077 Scenario: 10 Ensure PUT requests require an Id value When a PUT request is made to "/ed-fi/schools/" with """ @@ -339,8 +338,9 @@ Feature: Validation of the structure of the URLs "title": "Method Not Allowed", "status": 405, "correlationId": null, + "validationErrors": {}, "errors": [ - "Resource collections cannot be replaced. To \\"upsert\\" an item in the collection, use POST. To update a specific item, use PUT and include the \\"id\\" in the route." + "Resource collections cannot be replaced. To 'upsert' an item in the collection, use POST. To update a specific item, use PUT and include the 'id' in the route." ] } """ @@ -351,8 +351,7 @@ Feature: Validation of the structure of the URLs } """ - ## The resolution of this ticket will solve the execution error: https://edfi.atlassian.net/browse/DMS-353 - @API-078 @ignore + @API-078 Scenario: 11 Ensure DELETE requests require an Id value When a DELETE request is made to "/ed-fi/schools/" Then it should respond with 405 @@ -364,8 +363,9 @@ Feature: Validation of the structure of the URLs "title": "Method Not Allowed", "status": 405, "correlationId": null, + "validationErrors": {}, "errors": [ - "Resource collections cannot be deleted. To delete a specific item, use DELETE and include the \\"id\\" in the route." + "Resource collections cannot be deleted. To delete a specific item, use DELETE and include the 'id' in the route." ] } """