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

Added custom contract resolvers to handle STJ property name attributes #124

Merged
merged 11 commits into from Jan 5, 2023

Conversation

markphillips100
Copy link
Contributor

This PR fixes #113 by adding 2 new Json.Net contract resolvers to replace usage of existing DefaultContractResolver and CamelCasePropertyNamesContractResolver implementations. Their primary purpose being to retain the existing camel casing behaviour or not as required, with support for deserializing property names where the property is using a System.Text.Json.Serialization.JsonPropertyNameAttribute.

Some notes:

  1. Added some unit tests to validate the behaviour.
  2. Did not change the base type of PropertyIgnoreSerializerContractResolver as was unsure of the impact.
  3. Modified all ContractResolver assignments to use the appropriate new resolvers.
  4. There appears to be a broken unit test unrelated to this change, notably ValidateDateTimeFormatForDefaultDateTimeString().
  5. Added an msbuild property to build\AssemblyVersion.proj to allow me to not include my username in version. My local username ends in an underscore (no idea why) and this caused a problem building.

@markphillips100
Copy link
Contributor Author

@microsoft-github-policy-service agree [company="default"]

@markphillips100
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Collaborator

@hsubramanianaks hsubramanianaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markphillips100 Thank you for the PR. I appreciate it.
ValidateDateTimeFormatForDefaultDateTimeString - this UT is not failing in our CI/CD pipelines, and I manually tested them in my local which doesn't fail either. It Should be something related to your local machine.
we will review your changes further and let you know.

Thanks,
Hari

@markphillips100
Copy link
Contributor Author

@hsubramanianaks out of curiosity, are you (and your devops pipelines) in the UK? I'm east coast Australia and for my local windows 11 tests the DateTime.Parse(dateTimeString) used in the test will result in a local datetime which does not compare to the default(DateTime) used in the extension method under test.

If I convert the parsed datetime to universal time then the extension works as intended:

[Fact]
public void ValidateDateTimeFormatForDefaultDateTimeStringWhenPassedUtcKind()
{
   var dateTimeString = default(DateTime).ToUniversalTime().ToString("s") + "Z";
   Assert.Equal("-", DateTimeExtensions.FormatDateTimeUtcAsAgoString(DateTime.Parse(dateTimeString).ToUniversalTime()));
}

@markphillips100
Copy link
Contributor Author

Is someone else going to merge the latest changes? Also, any chance this can be reviewed and released soon?

Copy link
Contributor

@elenavillamil elenavillamil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thank you very much for your PR. I have left some small comments. The main changes look good!

build/AssemblyVersion.proj Show resolved Hide resolved
src/routingmanager/Properties/launchSettings.json Outdated Show resolved Hide resolved
@elenavillamil
Copy link
Contributor

Is someone else going to merge the latest changes? Also, any chance this can be reviewed and released soon?

Hi @markphillips100, sorry for the delay we were focused on the switch to dotnet6. We have had some time in the past couple days to look at this PR and once we get green light from our testing team Monday we should be able to merge it. If you can remove the launchSettings.json file it would be ideal, but it is not a big issue if it stays.

build/setup-kubectl.sh Outdated Show resolved Hide resolved
@Tatsinnit Tatsinnit added the enhancement New feature or request label Dec 17, 2022
@elenavillamil elenavillamil merged commit e0fd584 into Azure:main Jan 5, 2023
elenavillamil added a commit to elenavillamil/Bridge-To-Kubernetes that referenced this pull request Jan 10, 2023
Azure#124)

* Added custom contract resolvers to handle STJ property name attributes

* Help from the PR Comments.

* Removing unwnated file checkin by OP.

* add ash shell for alpine back.

* remove unwanted file.

Co-authored-by: Mark Phillips <mark_phillips21@hotmail.com>
Co-authored-by: Hariharan Subramanian <105889062+hsubramanianaks@users.noreply.github.com>
Co-authored-by: elenavillamil <mariaelena.villamil.rodriguez@gmail.com>
Co-authored-by: Tatsinnit <Tatsinnit@users.noreply.github.com>
hsubramanianaks referenced this pull request in hsubramanianaks/Bridge-To-Kubernetes Feb 2, 2024
….NET.Test.Sdk-17.8.0

Bump Microsoft.NET.Test.Sdk from 17.3.0 to 17.8.0 in /src
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deserialization of restorationjob's patch.json fails to deserialize namespace value
5 participants