Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Cap recursion in ORM #2992

Merged
merged 16 commits into from
Apr 12, 2023
Merged

Conversation

tevoinea
Copy link
Member

@tevoinea tevoinea commented Apr 6, 2023

Summary of the Pull Request

What is this about?

Closes #2900
Closes #2901
Closes #2902

#2900

Considers 2 new cases in our Entity Conversion code:

  1. If the discriminator field name is the field we're currently trying to deserialize, we're guaranteed to end up in an infinite loop
  2. If for any other reason, we iterate more than 100 times while trying to convert an entity to a type

In both cases, we throw a new OrmShortCircuitInfiniteLoopException and stop deserialization.

#2901 & #2902

  • Add a LogTracer to the EntityConverter
  • When throwing an exception, include as many tags and information as possible to help pin down the faulting entity/fields

Validation Steps Performed

How does someone test & validate?

Added a test that demonstrates the issue where the discriminator field EventType is self referential.

I looked through our codebase and didn't find any situation where we have this self referential situation but it's possible for the issue to be coming from entities in our database -- which would be much harder to check for this self referential behavior. Though, if/when this issue happens again, because of the improved logging changes, we should be able to track down the offending etities.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2023

Codecov Report

Merging #2992 (64a07ab) into main (ace0ccc) will increase coverage by 0.04%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##             main    #2992      +/-   ##
==========================================
+ Coverage   29.02%   29.07%   +0.04%     
==========================================
  Files         304      304              
  Lines       36326    36360      +34     
==========================================
+ Hits        10545    10570      +25     
- Misses      25781    25790       +9     
Impacted Files Coverage Δ
...rvice/ApiService/onefuzzlib/orm/EntityConverter.cs 90.25% <71.42%> (-2.34%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@Porges Porges left a comment

Choose a reason for hiding this comment

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

A few small typos to fix

@tevoinea tevoinea requested a review from Porges April 12, 2023 12:47
@tevoinea tevoinea enabled auto-merge (squash) April 12, 2023 13:38
@tevoinea tevoinea merged commit 41fa0a7 into microsoft:main Apr 12, 2023
@AdamL-Microsoft AdamL-Microsoft mentioned this pull request May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants