> On April 19, 2016, 11 a.m., Shwetha GS wrote: > > typesystem/src/main/java/org/apache/atlas/typesystem/types/FieldMapping.java, > > line 99 > > <https://reviews.apache.org/r/45948/diff/3/?file=1348566#file1348566line99> > > > > Can we maintain this as Set<String> and check on just typeName? > > David Kantor wrote: > I don't understand how checking typeName would avoid the infinite > recursion. The problem use case is when structs reference each other. So > let's say there is a bi-directional reference between A and B. When A is > output, its attribute values are output. So then B is output, and when its > attributes are output, FieldMapping.output() is called recursively on A. We > need to detect that A is already in the process of being output to avoid the > infinite recursion. How does checking typeName help with this? Please > advise. > > Shwetha GS wrote: > In FieldMappingTest.testOutputReferenceableInstance(), even > ownerType.toString() throws stack overflow. So, thought you were addressing > type.tostring() > > For type.toString(), we can use typename. For instance.tostring(), can we > use id string, instead of full instance in the set
Thanks for pointing out the stack overflow on ownerType.toString(). This involves type system objects and does not go through the FieldMapping.output() and IDataType.output() code path for outputting instance data - it takes a completely different code path. So changing the "in process" arg on these methods to Set<String> would not address this bug. Here is the relevant call stack: ClassType(HierarchicalType<ST,T>).toString() line: 371 String.valueOf(Object) line: 1657 StringBuilder.append(Object) line: 194 AttributeInfo.toString() line: 65 SingletonImmutableList<E>.toString() line: 97 String.valueOf(Object) line: 1657 StringBuilder.append(Object) line: 194 ClassType(HierarchicalType<ST,T>).toString() line: 372 String.valueOf(Object) line: 1657 StringBuilder.append(Object) line: 194 AttributeInfo.toString() line: 65 SingletonImmutableList<E>.toString() line: 97 String.valueOf(Object) line: 1657 StringBuilder.append(Object) line: 194 ClassType(HierarchicalType<ST,T>).toString() line: 372 Also, for instance data, using the id string is not sufficient because structs that are not implementations of IReferenceableInstance won't have an id string. And for those structs that are IReferenceableInstance objects, if the structs are newly created in memory and have not been persisted, they won't have a meaningful id string. Such objects would all have the same id string, and result in false positives on the inProcess check. I have re-implemented the fix to maintain the inProcess set in thread local storage rather than passing it as an argument. This fix addresses the recursion issue for both types and instance data. - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45948/#review129505 ----------------------------------------------------------- On April 29, 2016, 7:24 p.m., David Kantor wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45948/ > ----------------------------------------------------------- > > (Updated April 29, 2016, 7:24 p.m.) > > > Review request for atlas. > > > Bugs: ATLAS-645 > https://issues.apache.org/jira/browse/ATLAS-645 > > > Repository: atlas > > > Description > ------- > > ATLAS-645: In FieldMapping.output(), avoid infinite recursion when > IReferenceableInstance and IStruct instances reference each other. > > > Diffs > ----- > > > typesystem/src/main/java/org/apache/atlas/typesystem/persistence/StructInstance.java > af62442bfe5daa221079207acf361e1316cab3ad > > typesystem/src/main/java/org/apache/atlas/typesystem/types/FieldMapping.java > 36149bafff80b68ce176e82dcacac87035459362 > > typesystem/src/main/java/org/apache/atlas/typesystem/types/HierarchicalType.java > 89fcea6828c9e23c28a60952c3e4ca27c0667494 > > typesystem/src/main/java/org/apache/atlas/typesystem/types/OutputStatus.java > PRE-CREATION > typesystem/src/main/java/org/apache/atlas/typesystem/types/StructType.java > 54e344f5d6322a00ac7825ee8964f43a1552dcbe > > typesystem/src/test/java/org/apache/atlas/typesystem/types/OutputStatusTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/45948/diff/ > > > Testing > ------- > > Ran all unit and integration tests with no regressions. Added test cases > > > Thanks, > > David Kantor > >
