Add public ctor for SubscriptionValidationResponse #21975
Conversation
JoshLove-msft
requested review from
ahamad-MS,
jfggdl,
jsquire,
Kishp01,
KrzysztofCwalina,
pakrym and
tg-msft
as code owners
ghost
added
the
| @@ -69,7 +70,8 @@ public HttpRequestProcessor(ILogger<HttpRequestProcessor> logger) | |||
| SubscriptionValidationResponse validationResponse = new(){ ValidationResponse = validationCode }; | |||
| var returnMessage = new HttpResponseMessage(HttpStatusCode.OK) | |||
| { | |||
| Content = new StringContent(JsonConvert.SerializeObject(validationResponse)) | |||
| // use System.Text.Json to leverage the custom converter so that the casing is correct. | |||
| Content = new StringContent(STJ.JsonSerializer.Serialize(validationResponse)) | |||
There was a problem hiding this comment.
Does this mean the behavior of JsonConvert.SerializeObject(validationResponse) differs before and after this change?
There was a problem hiding this comment.
No, previously we were using a ValidationResponse model defined in the extension library which had a JsonProperty attribute to get camel casing.
Now, we are using STJ converter which uses camel casing. I couldn't continue to use NewtonSoft and use the Event Grid SDK model. It seemed worth using the SDK model here to validate that it works.
There was a problem hiding this comment.
FWIW I tested it out and both Pascal and camelCase work fine when adding a subscription, but in order to maintain the same behavior as before (and to make the tests pass) it seems better to use camel case.
...t.Azure.WebJobs.Extensions.EventGrid/src/Microsoft.Azure.WebJobs.Extensions.EventGrid.csproj
Show resolved
Hide resolved
| /// <param name="validationResponse"> The validation response sent by the subscriber to Azure Event Grid to complete the validation of an event subscription. </param> | ||
| /// <returns> A new <see cref="SystemEvents.SubscriptionValidationResponse"/> instance for mocking. </returns> | ||
| [EditorBrowsable(EditorBrowsableState.Never)] | ||
| public static SubscriptionValidationResponse SubscriptionValidationResponse(string validationResponse = default) |
There was a problem hiding this comment.
Do we need to hide the factory? I get that you can use the constructor directly but it feels nice to be able to create any model from the model factory.
There was a problem hiding this comment.
We don't need to, but it would go against our pattern of only including factories for output models. Codegen no longer generates the model factory for this type, so I had to add it back manually.
/cc @tg-msft @KrzysztofCwalina
There was a problem hiding this comment.
Yes - the model factory only exists for constructing the otherwise unconstructible per the guidelines today: https://azure.github.io/azure-sdk/dotnet_introduction.html#dotnet-mocking-factory-builder
It would take nontrivial effort to design and implement this across all of Track 2 when there's already a way of accomplishing it now, so it's not a high priority to me.
Also, use this ctor in the Event Grid function extension