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

FeedIterator<T>.ReadNextAsync throws NullReferenceException with custom serializer #871

Closed
fuocor opened this issue Oct 2, 2019 · 16 comments

Comments

@fuocor
Copy link
Contributor

fuocor commented Oct 2, 2019

Describe the bug
FeedIterator.ReadNextAsync throws NullReferenceException with custom serializer.
The stream sent to the serializer is empty. `All the other Cosmos methos serialize fine and GetItemQueryStreamIterator works as does the default serializer.

To Reproduce

var families = new List<Family>();

var setIterator = container.GetItemQueryIterator<Family>(requestOptions: new QueryRequestOptions { MaxItemCount = 1 });
while (setIterator.HasMoreResults)
{
    foreach (var item in await setIterator.ReadNextAsync())
    {
        families.Add(item);
    }
}

Expected behavior
Deserialized object.

Actual behavior
NullReferenceException

Environment summary
SDK Version: 3.0.100
Cosmos Version: 3.2.0
OS Version: Windows 10

@j82w
Copy link
Contributor

j82w commented Oct 2, 2019

Can you please provide the full exception message?

Query does return empty pages. If you have a custom serializer it should be able to handle this.

@fuocor
Copy link
Contributor Author

fuocor commented Oct 2, 2019

I tracked it down, The FeedIterator wraps the result inside a response with a "Documents" element. The default serializer seems to address it whereas a custom serializer needs to drill into the response

@j82w
Copy link
Contributor

j82w commented Oct 2, 2019

Here is what the contract should look like.

            // The stream contract should return the same contract as read feed.
            // {
            //    "_rid": "qHVdAImeKAQ=",
            //    "Documents": [{
            //        "id": "03230",
            //        "_rid": "qHVdAImeKAQBAAAAAAAAAA==",
            //        "_self": "dbs\/qHVdAA==\/colls\/qHVdAImeKAQ=\/docs\/qHVdAImeKAQBAAAAAAAAAA==\/",
            //        "_etag": "\"410000b0-0000-0000-0000-597916b00000\"",
            //        "_attachments": "attachments\/",
            //        "_ts": 1501107886
            //    }],
            //    "_count": 1
            // }

@j82w
Copy link
Contributor

j82w commented Oct 2, 2019

@fuocor I'm closing this issue since you figured out the issue. Please re-open it the issue isn't fixed.

@j82w j82w closed this as completed Oct 2, 2019
@fuocor
Copy link
Contributor Author

fuocor commented Oct 2, 2019

It makes writing a custom serializer nearly impossible. The type requested from the serializer is CosmosFeedResponseUtil<T>. How do you expect a custom serializer to instantiate that?

@j82w j82w reopened this Oct 2, 2019
@j82w
Copy link
Contributor

j82w commented Oct 2, 2019

That's a good point. The test have access to the internals which is why it's not having an issue with this.

The only option I see is to make CosmosFeedResponseUtil<T> public. Do you see any other alternatives?

@fuocor
Copy link
Contributor Author

fuocor commented Oct 2, 2019

Ask for an interface or abstract base class.

@fuocor
Copy link
Contributor Author

fuocor commented Oct 2, 2019

You have similar problems with ChangeFeed Processing.

 ResponseMessage.EnsureSuccessStatusCode()
    CosmosResponseFactory.ToObjectInternal[T](ResponseMessage cosmosResponseMessage, CosmosSerializer jsonSerializer)
    CosmosResponseFactory.<CreateItemResponseAsync>b__6_0[T](ResponseMessage cosmosResponseMessage)
    CosmosResponseFactory.ProcessMessageAsync[T](Task`1 cosmosResponseTask, Func`2 createResponse)
    DocumentServiceLeaseStoreCosmos.MarkInitializedAsync()
    BootstrapperCore.InitializeAsync()
    BootstrapperCore.InitializeAsync()
    PartitionManagerCore.StartAsync()
    ChangeFeedProcessorCore`1.StartAsync()

@j82w
Copy link
Contributor

j82w commented Oct 2, 2019

ResponseMessage and ItemResponse are public. It shouldn't have any issue creating an instance of these.

@fuocor
Copy link
Contributor Author

fuocor commented Oct 2, 2019

It's the content of the response that is the problem DocumentServiceLeaseCore and LockDocument

@j82w
Copy link
Contributor

j82w commented Oct 3, 2019

DocumentServiceLeaseCore and LockDocument shouldn't be using the custom serializer. It should be using the property serializer. That's definitely a bug. Fixing that seems like it's going to be interesting.

@joshlang
Copy link
Contributor

FYI: Using cosmos change feed is completely blocked for us until this is fixed. We can't deserialize CosmosFeedResponseUtil.

@j82w
Copy link
Contributor

j82w commented Nov 20, 2019

@ealsur does PR #944 fix this issue?

@ealsur
Copy link
Member

ealsur commented Nov 20, 2019

Yes, it fixes using a custom serializer with Change Feed, as it will only used for the Delegate changes.

@j82w
Copy link
Contributor

j82w commented Nov 20, 2019

We should be doing a release later this week or early next week. Just waiting on a few more PRs to get merged first.

@joshlang
Copy link
Contributor

That timeline will work for us. Thanks!

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

No branches or pull requests

4 participants