Skip to content

DATAMONGO-1005 - Improve cycle-detection for DbRef's. #209

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

Closed
wants to merge 3 commits into from

Conversation

thomasdarimont
Copy link

Introduced ObjectPath that collects target objects along the current property path while converting a DBObject to a domain object. If we detect that a potentially nested DBRef points to an object that is already under construction we simply return a reference to that object in order to avoid StackOverFlowErrors otherwise we resolve the DBRef.

thomasdarimont pushed a commit that referenced this pull request Jul 24, 2014
Introduced ObjectPath that collects the target objects while converting a DBObject to a domain object. If we detect that a potentially nested DBRef points to an object that is already under construction we simply return a reference to that object in order to avoid StackOverFlowErrors.

Original pull request: #209.
@odrotbohm
Copy link
Member

Generally looks good to me. A few observations:

  • I think it's fine to make ObjectPath amore ubiquitous type in the nested method calls to avoid the extracting and rewrapping of the current object.
  • what's in getObjectFromPathForRefOrNull(…) could be in ObjectPath directly. I also think we should make the lookup of the id value a operation that happens when adding the object to the ObjectPath as otherwise this will have to happen for every check. We might even be able to avoid the BeanWrapper-call entirely as the id value might have been resolved at the point in time we add the object to the path.
  • I don't entirely oversee the effects of creating new ObjectPath instances for every push. As a read operation is inherently single threaded we could might be able to just modify the instance handed around.

@thomasdarimont
Copy link
Author

I applied most of your suggestions, but I still create ObjectPath instances since I it is intended to be used as an immutable type.

Thomas Darimont added 3 commits August 5, 2014 15:08
Introduced ObjectPath that collects the target objects while converting a DBObject to a domain object. If we detect that a potentially nested DBRef points to an object that is already under construction we simply return a reference to that object in order to avoid StackOverFlowErrors.

Original pull request: #209.
Made ObjectPath a more ubiquitous type in the nested method calls to avoid the extracting and rewrapping of the current object. Introduced ObjectPathItem that captures the object, idValue and collection tuple to avoid performing the property lookup multiple times.
thomasdarimont pushed a commit that referenced this pull request Aug 12, 2014
Introduced ObjectPath that collects the target objects while converting a DBObject to a domain object. If we detect that a potentially nested DBRef points to an object that is already under construction we simply return a reference to that object in order to avoid StackOverFlowErrors.

Original pull request: #209.
odrotbohm added a commit that referenced this pull request Aug 12, 2014
Simplified implementation of ObjectPath to use a static root instance and hand the path further down until final resolution in MappingMongoConverter.readValue(…). This removes a bit of boxing and unboxing code both in ObjectPath and the converter.

Introduced ObjectPath.getPathItem(…) to internalize the iteration to find a potentially already resolved object.

Renamed parameters and fields of type ObjectPath to path consistently. Removed obsolete method in MappingMongoConverter.

Original pull request: #209.
@odrotbohm odrotbohm closed this Aug 12, 2014
@odrotbohm odrotbohm deleted the issue/DATAMONGO-1005 branch August 12, 2014 06:12
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.

2 participants