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
fix: Stack overflow on multi maps (and other non-trivial generic clas… #4501
Conversation
It seems that sundrio 0.93.1 is not yet fully released. I will make it Ready for review once it is. |
f8acda3
to
bf5df02
Compare
Sundrio has been released (thanks @iocanel), this is now ready for review. |
bf5df02
to
ba34cec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good, thanks a lot @xRodney !
I added one question and if you can find more tests to cover possible edge cases it would be nice (e.g. different number of generic type parameters etc. etc.).
crd-generator/api/src/test/java/io/fabric8/crd/example/map/ContainingMapsSpec.java
Outdated
Show resolved
Hide resolved
ba34cec
to
9bf6fc2
Compare
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
||
class ContainingMapsCRDTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these tests could be refactored to use single assertions with AssertJ, but it's not big deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this is probably a port of CRDGeneratorTest
. That one needs some refactoring too 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (codewise)
…es), schema for multimaps is wrong Logic in sundrio that is responsible for replacing generic type arguments on methods and properties with their concrete instantiations from subclasses contained a bug, in which it looped infinitely if the same type argument name was used at multiple classes in the hierarchy. A common occurence were multimaps. A very similar code was also present in crd-generator, also suffering from the same bug. Sundrio has been updated to account for this situation, and updated utilities from sundrio have been introduced to crd-generator, which replace the previous implementation of the same algorithm. A test has been added to validate that multimaps (like `class MultiMap<K,V> implements Map<K,List<V>>`) are generated correctly. Fixes fabric8io#4357 Fixes fabric8io#4487
9bf6fc2
to
0156d0f
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Thanks a lot for this fix @xRodney ! Much appreciated!
Description
…ses), schema for multimaps is wrong
Logic in sundrio that is responsible for replacing generic type arguments on methods and properties with their concrete instantiations from subclasses contained a bug, in which it looped infinitely if the same type argument name was used at multiple classes in the hierarchy. A common occurence were multimaps.
A very similar code was also present in crd-generator, also suffering from the same bug.
Sundrio has been updated to account for this situation, and updated utilities from sundrio have been introduced to crd-generator, which replace the previous implementation of the same algorithm.
A test has been added to validate that multimaps (like
class MultiMap<K,V> implements Map<K,List<V>>
) are generated correctly.Fixes #4357
Fixes #4487
Supersedes closes #4504
Type of change
test, version modification, documentation, etc.)
Checklist