-
Notifications
You must be signed in to change notification settings - Fork 451
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
Fix for ISO string modification during function metadata binding parsing #10735
base: dev
Are you sure you want to change the base?
Conversation
Discussed offline with @liliankasem - to incorporate suggestions we will need to refactor |
src/WebJobs.Script/Shared.cs
Outdated
|
||
namespace Microsoft.Azure.WebJobs.Script | ||
{ | ||
internal static class Shared |
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.
I don't love this class. The name is really vague. Could you create a folder called Serializers
and have a file with both of these classes in it?
Also do the settings need to be their own separate class?
how about something like this?
using Newtonsoft.Json;
namespace Microsoft.Azure.WebJobs.Script.Serialization
{
/// <summary>
/// Provides shared JSON serialization settings and serializers.
/// </summary>
internal static class JsonSerialization
{
/// <summary>
/// JSON serializer settings with no date parsing.
/// </summary>
public static readonly JsonSerializerSettings NoDateParsingSettings = new JsonSerializerSettings
{
DateParseHandling = DateParseHandling.None
};
/// <summary>
/// JSON serializer instance configured with no date parsing settings.
/// </summary>
public static readonly JsonSerializer NoDateParsingSerializer =
JsonSerializer.Create(NoDateParsingSettings);
}
}
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.
Second I am concerned about the thread safety of this, and testability?
Is there a second we didn't go with DI for this?
@@ -1,24 +0,0 @@ | |||
// Copyright (c) .NET Foundation. All rights reserved. |
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.
I think the file needs to be deleted?
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-proc
branch to be included in Core Tools and non-Flex deployments.in-proc
branch is not requiredrelease_notes.md
Additional information
Tested locally E2E the following scenarios:
StartFromTime
set to a past time started triggering and processing existing records immediately after startup (older records than start time untouched)StartFromTime
set 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).