Skip to content
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

add missing JsonPropertyName to Data property #1000

Closed
wants to merge 7 commits into from

Conversation

NenadSteric
Copy link

@NenadSteric NenadSteric commented Dec 20, 2022

without it you need to force the JsonSerializer to use camelCase with JsonSerializerOptions

Description

adds JsonPropertyName to make the casing consistent with the other properties.

This also makes it possible to use just Serialize without JsonSerializerOptions
as the pubsub json handling code in Dapr needs this property to be lowercase.

This problem had to be fixed during testing of the new Bulk-publish API
where I needed to serialize the CloudEvent classes manually without the DaprClient.

Issue reference

as discussed in
#976 and #944

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests - no new tests necessary, existing ones are green
  • Extended the documentation - fixes an oversight - wouldnt know where to document this

Please comment if any new tests or documentation needs to be created.

@NenadSteric NenadSteric requested review from a team as code owners December 20, 2022 07:29
@NenadSteric NenadSteric changed the title [WIP] add missing JsonPropertyName to Data property add missing JsonPropertyName to Data property Dec 22, 2022
@codecov
Copy link

codecov bot commented Dec 22, 2022

Codecov Report

Patch and project coverage have no change.

Comparison is base (99d874a) 66.42% compared to head (d698577) 66.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1000   +/-   ##
=======================================
  Coverage   66.42%   66.42%           
=======================================
  Files         171      171           
  Lines        5758     5758           
  Branches      626      626           
=======================================
  Hits         3825     3825           
  Misses       1784     1784           
  Partials      149      149           
Flag Coverage Δ
net6 66.42% <ø> (ø)
net7 66.42% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/Dapr.Client/CloudEvent.cs 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NenadSteric
Copy link
Author

@approvers-dotnet-sdk: anything else needed ?

@NenadSteric NenadSteric force-pushed the fix_JsonSerialization branch 2 times, most recently from 008e7df to 92ad699 Compare January 30, 2023 20:48
@WhitWaldo
Copy link
Contributor

@NenadSteric Thank you for your contribution!

Upon digging into this, the various Dapr clients utilize a JsonSerializerOptions initialized with new JsonSerializerOptions(JsonSerializerDefaults.Web). One of the characteristics of the Web defaults is the use of "camelCase" name formatting, meaning that even without the JsonPropertyName attribute, the default value used will be "dapr" even without specifying it as much.

Further, because a JsonPropertyName would override the JsonSerializerOptions, merging your PR would result in developers being unable to trivially change the name format even if they really wanted to for some reason.

As such, I'm going to go ahead and reject this PR because as the underlying intent is to use a camel-cased name and it's already doing that by default and in an easily overridable manner, we're already at the desired outcome without any changes at all.

That said, if you think I'm missing something, please feel free to re-open with any follow-ups and I'd be happy to re-engage!

@WhitWaldo WhitWaldo closed this Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants