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

fix: start context in MicronautRequestStreamHandler #1985

Merged
merged 1 commit into from
Dec 13, 2023
Merged

Conversation

sdelamo
Copy link
Contributor

@sdelamo sdelamo commented Dec 13, 2023

Close: #1913

This PR changes so the MicronautRequestStreamHandler constructor starts the application context, and it invokes inject(this) for the handler to resolve injection points.

Close: #1913

This PR changes so the MicronautRequestStreamHandler constructor starts the application context, and it invokes `inject(this)` for the handler to resolve injection points.
@sdelamo sdelamo added the type: bug Something isn't working label Dec 13, 2023
Copy link

sonarcloud bot commented Dec 13, 2023

Copy link
Member

@timyates timyates left a comment

Choose a reason for hiding this comment

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

Is it worth changing MicronautRequestStreamHandler so it's exactly the same as MicronautRequestHandler?

diff --git a/function-aws/src/main/java/io/micronaut/function/aws/MicronautRequestStreamHandler.java b/function-aws/src/main/java/io/micronaut/function/aws/MicronautRequestStreamHandler.java
index 809c7a3f4..d5dc3f6ae 100644
--- a/function-aws/src/main/java/io/micronaut/function/aws/MicronautRequestStreamHandler.java
+++ b/function-aws/src/main/java/io/micronaut/function/aws/MicronautRequestStreamHandler.java
@@ -55,9 +55,13 @@ public class MicronautRequestStreamHandler extends StreamFunctionExecutor<Contex
      * Lambda deployment.
      */
     public MicronautRequestStreamHandler() {
-        buildApplicationContext(null);
-        startEnvironment(applicationContext);
-        injectIntoApplicationContext();
+        try {
+            buildApplicationContext(null);
+            injectIntoApplicationContext();
+        } catch (Exception e) {
+            LOG.error("Exception initializing handler", e);
+            throw e;
+        }
     }

     /**
@@ -66,8 +70,14 @@ public class MicronautRequestStreamHandler extends StreamFunctionExecutor<Contex
      */
     public MicronautRequestStreamHandler(ApplicationContext applicationContext) {
         this.applicationContext = applicationContext;
-        startEnvironment(applicationContext);
-        injectIntoApplicationContext();
+
+        try {
+            startEnvironment(applicationContext);
+            injectIntoApplicationContext();
+        } catch (Exception e) {
+            LOG.error("Exception initializing handler: " + e.getMessage() , e);
+            throw e;
+        }
     }

     private void injectIntoApplicationContext() {
@@ -91,12 +101,9 @@ public class MicronautRequestStreamHandler extends StreamFunctionExecutor<Contex

     @Override
     protected ApplicationContext buildApplicationContext(Context context) {
-        try {
-            return super.buildApplicationContext(context);
-        } catch (Exception e) {
-            LOG.error("Exception initializing handler: " + e.getMessage(), e);
-            throw e;
-        }
+        applicationContext = super.buildApplicationContext(context);
+        startEnvironment(applicationContext);
+        return applicationContext;
     }

     @NonNull

It might make future comparison easier?

Copy link
Member

@timyates timyates left a comment

Choose a reason for hiding this comment

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

Actually, I think the code suggested above is more correct... It sets this.applicationContext which I cannot see happening in this PR?

@timyates
Copy link
Member

Ahhh, I see... that happens in super.buildApplicationContext

@sdelamo sdelamo merged commit 426909e into 4.1.x Dec 13, 2023
15 checks passed
@sdelamo sdelamo deleted the issue-1913 branch December 13, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MicronautRequestStreamHandler is missing ApplicationContext initialisation and startup
2 participants