-
Notifications
You must be signed in to change notification settings - Fork 38
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: remove thread local based wiring #1817
Conversation
da2d9f5
to
57513f1
Compare
@Component | ||
public class IllDefinedWorkflow extends Workflow<String> { |
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.
This class and the one below are not related to the fix, but we were missing this test for Worklows
// NOTE: if they are allowed, 'partial' should already have a matching case for them | ||
case p if p == classOf[KalixClient] => | ||
new BeanCreationException( | ||
s"KalixClient cannot be inject into a [${constructor.getDeclaringClass.getSimpleName}]") |
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.
It should just create and return the exception, not throw it?
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.
Oh god! I don't know how to code anymore.
// block wiring of clients into anything that is not an Action or Workflow | ||
// NOTE: if they are allowed, 'partial' should already have a matching case for them | ||
case p if p == classOf[KalixClient] => | ||
throw new BeanCreationException( |
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.
Look @johanandren, now throwing for real.
// NOTE: if they are allowed, 'partial' should already have a matching case for them | ||
case p if p == classOf[KalixClient] => | ||
throw new BeanCreationException( | ||
s"[${constructor.getDeclaringClass.getSimpleName}] are not allowed to have a dependency on KalixClient") |
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.
I re-phrased the text here because I was using the article 'a' in previous commit. It would be wrong for EventSourcedEntity.
if (bean == null) | ||
throw new BeanCreationException( | ||
s"Cannot wire [${anyOther.getSimpleName}]. Bean not found in the Application Context"); |
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.
I also remove the check on wiring wrong context.
This more generic check will cover all wrong contexts, but also cases in which the user didn't properly define their own bean.
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.
We can go one extra mile and provide specific error messages when a user tries to inject a ActionContext into an EventSourcedEntity, for example.
But maybe that's something we should do using our Validation frameworky.
Fix #1812