[ODS-5581] Bring dynamic profile spike work to current codebase #581
Conversation
…n already implemented for DataManagementRequestContext.
…ted boilerplate version already implemented for DataManagementRequestContext.
…ion on code repository path string parsing failures.
…support for Profiles.
…ngle static FullName value (rather than creating a new one every time).
…for entity extensions.
…mblies without the assembly metadata JSON file.
| : 0); | ||
|
|
||
| hashCode = (hashCode * 397) ^ ProfileResourceName.GetHashCode(); | ||
| hashCode = (hashCode * 397) ^ (int)ContentTypeUsage; |
There was a problem hiding this comment.
Any reason not to use System.HashCode instead?
There was a problem hiding this comment.
This undoubtedly was generated by Rider/ReSharper. Are you suggesting not implementing the method at all, or are you referring to the cast to int on the ContentTypeUsage?
There was a problem hiding this comment.
FYI: I just ran a Linqpad expression and 7.GetHashCode() returns 7.
There was a problem hiding this comment.
I meant using the new HashCode.Combine to calculate the hash code, the logic is the same but it produces cleaner code. ReSharper already uses it by default.
I'm good with the current implementation, I was just wondering if there were any reasons not to use HashCode.Combine.
Application/EdFi.Ods.Common/Models/ResourceItemPredicateBuilder.cs
Outdated
Show resolved
Hide resolved
Utilities/CodeGeneration/EdFi.Ods.CodeGen/Mustache/EntityInterfaces.mustache
Show resolved
Hide resolved
...odeGeneration/EdFi.Ods.CodeGen.Tests/IntegrationTests/Providers/AssemblyDataProviderTests.cs
Show resolved
Hide resolved
...s/CodeGeneration/EdFi.Ods.CodeGen.Tests/UnitTests/Processing/TemplateContextProviderTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Since we no longer generate Profile controllers, GetReadContentType doesn't get overridden, so the response content-type is always application/json regardless if it's a Profile request.
We can check the ProfileContentTypeContext to return the appropriate content-type.
| return ContentTypeUsage.Writable; | ||
| } | ||
|
|
||
| throw new BadRequestException($"The profile usage segment in the profile-based '{headerName}' header was not recognized."); |
There was a problem hiding this comment.
This returns HTTP 500 instead of 400. I didn't test the other errors but most likely have the same behaviour.
There was a problem hiding this comment.
So the work to address this goes a little deeper than we need to for this spike ticket. This middleware is running at a much lower level in the processing than the fallback exception translation (the ExceptionHandlingFilter).
I've added a comment to the ticket for updating the Profiles Postman tests to ensure we don't forget to address this when it comes time to work on the ticket to prepare the branch for final merge to main with ODS-5611.
There was a problem hiding this comment.
I have added Postman tests in the "Profiles" test suite covering these scenarios, specifically:
- Expecting a 400 response status with the exception message above in the
messageproperty of the response (for both read and write requests) when the content type usage is not eitherreadableorwritable. - Expecting a profile-specific
Content-Typeresponse header for both Get All and Get By Id requests (Get By Id scenario fails against currentmainbranch).
There was a problem hiding this comment.
Covering tests have been moved into the main Profiles Suite now, and are passing (with commits added to ODS-5610 branch, which is based on this one).
No description provided.