Skip to content

Conversation

brphelps
Copy link
Contributor

@brphelps brphelps commented Feb 7, 2020

When using a "watermark" (e.g. updatedAt) as a predicate to a query, the library produces stack overflow exceptions because it repeatedly tries to serialize the same object. There should probably be an additional change to make sure that that loop doesn't happen and throw a StackOverflow, maybe instead saying "Serializing this type is not supported".

Any tips on where I should put the testing for this would be helpful, it looks like this class doesn't have unit tests specific to just it.

Adding support for DTO in the serialize method.
@codecov
Copy link

codecov bot commented Feb 7, 2020

Codecov Report

Merging #214 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #214      +/-   ##
==========================================
+ Coverage   79.25%   79.28%   +0.02%     
==========================================
  Files          73       73              
  Lines        2473     2476       +3     
  Branches      354      355       +1     
==========================================
+ Hits         1960     1963       +3     
  Misses        436      436              
  Partials       77       77              
Impacted Files Coverage Δ
...t.GraphQL.Core/Core/Serializers/QuerySerializer.cs 95.00% <100.00%> (+0.09%) ⬆️

Copy link
Collaborator

@jcansdale jcansdale left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tests and everything! 🙏

@LordJZ
Copy link
Contributor

LordJZ commented Apr 6, 2020

Can this be merged and released please?

@brphelps
Copy link
Contributor Author

brphelps commented Apr 7, 2020

Sorry for the wait @LordJZ -- circling back on the PR now.

@brphelps brphelps linked an issue Apr 9, 2020 that may be closed by this pull request
Forcing policy re-evaluation
@brphelps brphelps merged commit 36ebb1c into octokit:master Apr 27, 2020
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.

Using DateTimeOffset parameters causes a StackOverflowException
3 participants