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.

Sign up for GitHub

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

Jump to bottom

Fix customer serializer being used for internal types #1105

Merged
merged 15 commits into from
Dec 19, 2019
Merged

Fix customer serializer being used for internal types #1105

merged 15 commits into from
Dec 19, 2019

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Dec 13, 2019 •
edited
Loading

Pull Request Template

Description

This change fixes a bug where the custom serializer is being used to convert several internal types. The custom serializer can not convert the internal types and causes exception.

To fix the issue:

  1. Refactored the different serializes into a single object CosmosSerializerCore .
  2. CosmosSerializerCore will decide which serializer to use based on the type of the object.
  3. CosmosSerializerCore has debug checks to break if an internal type is being converted by the custom serializer.
  4. Replaced CosmosFeedResponseUtil with CosmosElements to unwrap the service response. This avoid exposing a type to the public and can support lazy deserialization.
  5. This would normally would be considered a breaking change because the serialization is always a single item vs an array before. Since CosmosFeedResponseUtil is internal none of the array logic previously worked. Making this a non-breaking change.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Closes #1004, #871, #1106

@j82w j82w added the bug Something isn't working label Dec 13, 2019
@j82w j82w self-assigned this Dec 13, 2019
@j82w j82w mentioned this pull request Dec 13, 2019
Closed
@j82w j82w requested a review from ealsur December 16, 2019 17:03
kirankumarkolli
kirankumarkolli reviewed Dec 17, 2019
View reviewed changes
changelog.md Outdated Show resolved Hide resolved
ealsur
ealsur reviewed Dec 17, 2019
View reviewed changes
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Tests/ResultSetIteratorTests.cs
operationType: 'Replace',
resourceType: 'trigger'
string conflictResponsePayload = @"{ ""Conflicts"":[{
""id"": ""Conflict1"",
Copy link
Member

Choose a reason for hiding this comment

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

Is the change to double escaping required? Why is that?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I understand the validity concern, but did anything that that would make previous format not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Single quotes is not valid JSON. It seems like newtonsoft will still convert it, but CosmosElements will not. The backend always send valid json with doube quotes.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is also something that will happen with System.Text.Json and anyone passing invalid JSON

ealsur
ealsur reviewed Dec 17, 2019
View reviewed changes
Microsoft.Azure.Cosmos/src/Serializer/CosmosSerializerCore.cs
/// <summary>
/// This is an interface to allow a custom serializer to be used by the CosmosClient
/// </summary>
internal class CosmosSerializerCore
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it inherit from CosmosSerializer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't want the public contract restricting the internal type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you can create CosmosSerializerInternal that inherits from CosmosSerializer and inherit from that instead.

In reply to: 359071623 [](ancestors = 359071623)

ealsur
ealsur reviewed Dec 17, 2019
View reviewed changes
Copy link
Member

@ealsur ealsur left a comment

Choose a reason for hiding this comment

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

My only big question is, in all the internal scenarios where we are now passing CosmosSerializerCore instead of CosmosSerializer means that we won't be able to pass a mocked instance, right? Are we ok with that? Any gaps where we think we would mock the serializer?

@j82w
Copy link
Contributor Author

j82w commented Dec 18, 2019 •
edited
Loading

My only big question is, in all the internal scenarios where we are now passing CosmosSerializerCore instead of CosmosSerializer means that we won't be able to pass a mocked instance, right? Are we ok with that? Any gaps where we think we would mock the serializer?

Currently nothing is mocking the serializerCore so I didn't bother making the methods virtual. It's an internal class so we can modify it at any time without impacting users. This still support mocking the custom serializer and passing it in.

ealsur
ealsur previously approved these changes Dec 19, 2019
View reviewed changes
bchong95
bchong95 reviewed Dec 19, 2019
View reviewed changes
Microsoft.Azure.Cosmos/src/CosmosClient.cs
this.ClientContext.ResponseFactory.CreateQueryFeedResponse<T>);
(response) => this.ClientContext.ResponseFactory.CreateQueryFeedResponse<T>(
responseMessage: response,
resourceType: ResourceType.Database));
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is questionable right? Why do we make this generic when it always returns a database response that is schematized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't always return a DatabaseProperties. The user can specify a custom type.

Example query:
"Select t.id from t"

bchong95
bchong95 previously approved these changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@bchong95 bchong95 left a comment

Choose a reason for hiding this comment

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

:shipit:

@j82w j82w dismissed stale reviews from bchong95 and ealsur via b8093c2 December 19, 2019 19:56
@kirankumarkolli kirankumarkolli merged commit 36444e8 into master Dec 19, 2019
@kirankumarkolli kirankumarkolli deleted the users/jawilley/bug/serializer_internal branch December 19, 2019 20:35
@joshlang
Copy link
Contributor

Just grabbed your newly released 3.6.0 package.

HURRRRRRRRRRRAY!!!

Thank you :D :D

@j82w j82w mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirankumarkolli kirankumarkolli kirankumarkolli approved these changes

@bchong95 bchong95 bchong95 left review comments

@ealsur ealsur ealsur left review comments

@khdang khdang Awaiting requested review from khdang khdang is a code owner

@kirillg kirillg Awaiting requested review from kirillg kirillg is a code owner

Assignees

@j82w j82w

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NullReferenceException in FeedIterator.ReadNextAsync
5 participants
@j82w @joshlang @ealsur @bchong95 @kirankumarkolli