Core: resolveAll() must be called before reading resolution results #3064
Conversation
| // Set when resolveAll is called | ||
| private ResolverStatus primaryResolverStatus = null; | ||
|
|
||
| private ResolverStatus requirePrimaryResolverStatus() { |
There was a problem hiding this comment.
minor : require gives an impression that its just doing assertion, but its additionally returning status how about this
| private ResolverStatus requirePrimaryResolverStatus() { | |
| private ResolverStatus safeGetPrimaryResolverStatus() { |
There was a problem hiding this comment.
Refactored a bit. Thanks for the suggestion.
There was a problem hiding this comment.
Thx! The new code is much nicer 🙂
|
|
||
| public PolarisResolvedPathWrapper getResolvedReferenceCatalogEntity( | ||
| boolean prependRootContainer) { | ||
| requirePrimaryResolverStatus(); |
There was a problem hiding this comment.
[doubt] is just checking status not null sufficient ? or should we bake the assertion that that status != null and status = SUCCESS ?
There was a problem hiding this comment.
That's a good point. Removed this line to avoid any behavior change.
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
@flyrain : could you explain why "resolveAll() must be called before reading resolution results"? The PR description provides information about enforcing it, but the rationale for doing that is not very clear... at least to me 😅 Thx!
The result will be invalid, or null if it's not invoked. The concurrent code actually already enforce that. You can find the following code in each place reading the result, but NPE will be threw in case of |
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM thanks @flyrain !
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
3 participants
To fail fast and explicitly when resolveAll() isn't called before reading the results
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)