Fix for ISO string modification during function metadata binding parsing #10735
+32
−5
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
liliankasem
reviewed
Jan 11, 2025
| @@ -244,13 +247,26 @@ internal IEnumerable<FunctionMetadata> ValidateMetadata(IEnumerable<RawFunctionM | |||
| return validatedMetadata; | |||
| } | |||
|
|
|||
| private static BindingMetadata GetBindingMetadata(string binding) | |||
There was a problem hiding this comment.
Can this be CreateBindingMetadata ?
There was a problem hiding this comment.
nit: can the private method be underneath the internal methods?
| using (StringReader stringReader = new StringReader(binding)) | ||
| using (JsonTextReader jsonTextReader = new JsonTextReader(stringReader)) | ||
| { | ||
| JsonSerializer serializer = JsonSerializer.Create(_jsonSerializerSettings); |
There was a problem hiding this comment.
Can we DI the JsonSerializer so we're not creating one every time this method is called?
| @@ -28,6 +30,7 @@ internal class WorkerFunctionMetadataProvider : IWorkerFunctionMetadataProvider | |||
| private readonly IScriptHostManager _scriptHostManager; | |||
| private string _workerRuntime; | |||
| private ImmutableArray<FunctionMetadata> _functions; | |||
| private static JsonSerializerSettings _jsonSerializerSettings = new JsonSerializerSettings { DateParseHandling = DateParseHandling.None }; | |||
There was a problem hiding this comment.
Can we setup the serializer + settings as part of the ctor?
| @@ -10,3 +10,4 @@ | |||
| - [Added net9 prelaunch app.](https://github.com/Azure/azure-functions-dotnet-worker/pull/2898) | |||
| - Update the `DefaultHttpProxyService` to better handle client disconnect scenarios (#10688) | |||
| - Replaced `InvalidOperationException` with `HttpForwardingException` when there is a ForwarderError | |||
| - Fix the accidental modification of ISO strings during function metadata binding parsing. This fixes the listener errors related to the CosmosDB trigger's `StartFromTime` parameter. (#10735) | |||
There was a problem hiding this comment.
Suggested change
| - Fix the accidental modification of ISO strings during function metadata binding parsing. This fixes the listener errors related to the CosmosDB trigger's `StartFromTime` parameter. (#10735) | |
| - Fix modification of ISO strings during the parsing of function metadata bindings. | |
| - This fixes the listener errors related to the CosmosDB trigger's `StartFromTime` parameter. (#10735) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue describing the changes in this PR
resolves #10732 (and Azure/azure-functions-dotnet-worker#2882)
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.mdAdditional information
Tested locally E2E the following scenarios:
StartFromTimeset to a past time started triggering and processing existing records immediately after startup (older records than start time untouched)StartFromTimeset to a future date and will process any new changes following startup. Discussed with original engineers - this feature was only meant to go into the past, so there was never a design set for future dates. This setting will not ignore changes until that future date (note: need to test InProc to see if behavior is the same there).