-
Notifications
You must be signed in to change notification settings - Fork 5
EES-5874 Create Search Function App #5630
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.
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
Conversation
0a97142
to
332fc80
Compare
332fc80
to
bc6db15
Compare
src/GovUk.Education.ExploreEducationStatistics.Content.Search.FunctionApp/.gitignore
Outdated
Show resolved
Hide resolved
...rch.FunctionApp/GovUk.Education.ExploreEducationStatistics.Content.Search.FunctionApp.csproj
Outdated
Show resolved
Hide resolved
...rch.FunctionApp/GovUk.Education.ExploreEducationStatistics.Content.Search.FunctionApp.csproj
Show resolved
Hide resolved
...ation.ExploreEducationStatistics.Content.Search.FunctionApp/Clients/AzureBlobStorage/Blob.cs
Show resolved
Hide resolved
...ExploreEducationStatistics.Content.Search.FunctionApp/Clients/ContentApi/ContentApiClient.cs
Show resolved
Hide resolved
...ExploreEducationStatistics.Content.Search.FunctionApp/Clients/ContentApi/ContentApiClient.cs
Outdated
Show resolved
Hide resolved
throw new UnableToGetPublicationLatestReleaseSearchViewModelException(publicationSlug, e.StatusCode, e.Message); | ||
} | ||
|
||
var result = await response.Content.ReadFromJsonAsync<ReleaseSearchViewModelDto>(cancellationToken: cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just letting you know there could be a problem here as I think Content API responses are serialised using Newtonsoft.Json, but this is using System.Text.Json. They are not always compatible but it might just be the fields involved here are working fine. I seem to remember there's a particular incompatibility around enums and this has been a problem for the Public API where the Admin (which also uses Newtonsoft.Json) consumes data from the Public API which uses System.Text.Json.
We want to migrate away from Newtonsoft.Json in favour of System.Text.Json, but we haven't completed this yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be a configuration issue then. I'm hoping/praying that you're serialising the enums as their string label and not as some arbitrary number? There should be no compatibility issues as json serialisation is well defined and both implementations follow it. I'll double check to ensure the view model the API is producing is outputting the string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I've just checked and the view model does not have any enums on it.
I've also noticed your EnumToEnumValueJsonConverter. I'm sure this is a configuration setting and that we can set that instead. Let me see if I can find it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, mostly configuration issues that can either be resolved with out-of-the-box supported config or other workarounds.
There's a documented list of differences here in case you've not already seen it.
I think with enums it's easy to just add in the System.Text.Json string value converter, see enums-as-strings.
Our own EnumToEnumValueJsonConverter
is doing something different. Thanks for pointing it out because that's where the incompatibility around enums came from for the Public API that I was thinking of, where we've also had to write our own System.Text.Json variant of that converter. There's also EnumToEnumLabelJsonConverter
and the System.Text.Json variant of that.
'EnumToEnumValue' and 'EnumToEnumLabel' refer to getting the label or value from an EnumLabelValueAttribute
annotated enum entry. E.g. GeographicLevel.LocalAuthority is annotated with a label of 'LocalAuthority' and value of 'LA'. With hindsight I think we probably regret this custom implementation! 😂
[EnumLabelValue("Local authority", "LA")]
LocalAuthority,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes. Well, for the view model that I am consuming, there are no such gremlins. And if we were to add an enum value to view model, I would represent it as a string through an explicit conversion and not rely on the serialiser. The dto can be unit tested also to ensure that the json is deserialised correctly.
...ExploreEducationStatistics.Content.Search.FunctionApp/Clients/ContentApi/ContentApiClient.cs
Show resolved
Hide resolved
....ExploreEducationStatistics.Content.Search.FunctionApp/Services/SearchableDocumentCreator.cs
Outdated
Show resolved
Hide resolved
376af36
to
d3b73f7
Compare
....ExploreEducationStatistics.Content.Search.FunctionApp/Services/SearchableDocumentCreator.cs
Show resolved
Hide resolved
...rch.FunctionApp/GovUk.Education.ExploreEducationStatistics.Content.Search.FunctionApp.csproj
Outdated
Show resolved
Hide resolved
src/GovUk.Education.ExploreEducationStatistics.Content.Search.FunctionApp/local.settings.json
Outdated
Show resolved
Hide resolved
…est release with the associated metadata Move ReleaseSearchViewModel to own file. Code tidy. WIP Putting together the new Content Processor for Search Rename releaseId to releaseVersionId rename releaseId to releaseVersionId in Content Processor code tidy Ensure the summary is made safe for upload Ensure Title is also made ascii and no spaces. Also, replace the magic strings Code tidy Add Azure Blob Storage Client integration tests. Also added a blob download and delete methods. Add a couple more tests for Download and Delete non-existent blobs Add tests for HostBuilder. Flow cancellation token through everywhere. Add tests for SearchableDocumentCreator Rename the search Processor to Search.FunctionApp Moved projects to Search/Function App solution folder Mask connection string Code tidy Getting the function app set up Test config and container registrations set up If service is not configured, fail on startup instead of waiting for a message to arrive Fix config Severed the ref to viewmodels. Added dto. Code Tidy. Removed unused refs. Remove the hardcoded queue names from the trigger function and move them to config Add the local.settings.json file Rename the function Add ReleaseId to the searchable view model to use as the blob name Fix Fix Fix merge Amendments made based on PR feedback Moved Azure Storage code that was only being used by the integration tests into the test project to clean up the client. Add Search Function App to the build yaml Add search function app to the start.ts useful scripts Remove config for durable storage as we have gone with the non-durable approach Make the metadata keys ALL-CAPS-KEBABS. Also, rename Type to ReleaseType for added clarity.
bc921b1
to
8056d78
Compare
New Azure Function App for Search
This PR contains a new function app created to handle new search processes.
Currently, there is only a single Azure Function called CreateSearchableReleaseDocument.
CreateSearchableReleaseDocument Function
This Azure Function is triggered by a new message on a source queue which contains details of a new Release version being published.
It consumes the message and makes a call to the ContentAPI to retrieve a ReleaseSearchViewModel. This contains a searchable HTML version of the release, plus associated metadata.
The function then creates a new Blob in the searchable documents container with this information.
Finally, the function publishes a message about the new Searchable Document to an output queue.