Skip to content
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

Di pipeline #10

Open
wants to merge 2 commits into
base: kaibocai/dc-middleware
Choose a base branch
from

Conversation

patriot1burke
Copy link

@patriot1burke patriot1burke commented Sep 13, 2022

Extended FunctionWorkerMiddleware to provide hooks for dependency injection frameworks. See #642

public ClassLoader getFunctionClassLoader();
public Class getFunctionClass();

public Object getFunctionInstance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Quarkus, we don't need getFunctionClassLoader and getFunctionInstance right. Please correct me if I am wrong.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFunctionClassLoader() is removable so long as getFunctionClass().getClassLoader() returns the correct loader.

getFunctionInstance() you'd want to have to see if another middleware has overriden the instance already and proxy that instance, output a warning, or abort. An alternative to that is for setFunctionInstance() to throw an exception if the instance has already been set. What do you think is the best approach?

Copy link
Member

@kaibocai kaibocai Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense, I was just wondering if we would have such a use case. The only middleware that require access to FunctionInstance will be DI framework like Quarkus or Spring, etc. But cx will not use both Quarkus and Spring at the same time.
However, I can see this is valid point. I will explore this more from our side.
Personally, I prefer second direction has setFunctionInstance() do the check and throw the exception if this case happens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

middleware would be pulled in via a maven dependency. So, if an app developer accidently pulled in both a Quarkus and Spring middleware dependency by accident (or on purpose) you could have conflict. These sort of dependency conflicts are more common than you think based on my experience.

Not that important though. Just something to think about.

@kaibocai
Copy link
Member

Hi @patriot1burke , thanks very much for your contribution. We actually get other third parties asking for middleware support. We currently plan to provide all required APIs in interface ExecutionContext (All APIs require by you will also be there). We will let you know if we decide to separate a dedicate interface only for middleware usage. Thank you.

@patriot1burke
Copy link
Author

@kaibocai Here's a Quarkus extension that uses these APIs. I can update it to point to a separate branch you've developed if you provide similar hooks:

https://github.com/patriot1burke/quarkus/tree/azure-functions-extension/extensions/azure-functions

@patriot1burke
Copy link
Author

@kaibocai Has the FunctionInstanceInjector SPI been released yet? I'm getting correct callback for middleware, but not for the instance injector.

@kaibocai
Copy link
Member

kaibocai commented Jan 31, 2023

Hi @patriot1burke , the DI hook is fully released on Linux. You can try to test it by creating Linux function app. For Windows, it may still need one week or two to fully released to all regions. Thanks.

https://mvnrepository.com/artifact/com.microsoft.azure.functions/azure-functions-java-spi/1.0.0 maven repo link

@kaibocai
Copy link
Member

@kaibocai Has the FunctionInstanceInjector SPI been released yet? I'm getting correct callback for middleware, but not for the instance injector.

Hi @patriot1burke , the DI hook feature is fully released on production for both Windows and Linux. We are releaseing the local core tools in next one week or two. Will update you once it's released. Thanks.

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