Fix class loader issue for notifications response

[joshuali925]

Signed-off-by: Joshua Li joshuali925@gmail.com

Description

fix from @dai-chen:
Recreate the response object from ActionResponse as specific notifications response in notifications plugin interface to avoid classloader issue when passing response between plugins

Issues Resolved

#37

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dai-chen]

Thanks for the fix!

[dblock]

I love this as a start.

Try to refactor this where you don't need to override fun onResponse manually in every instance.

I think I want to be able to write this:

wrapper.execute(
    SEND_NOTIFICATION_ACTION_TYPE,
    SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),
    object: SomeNewType<ActionResponse, SendNotificationResponse>
}

[dblock]

This should be refactored, and definitely needs tests to be merged.

[dai-chen]

I love this as a start.

Try to refactor this where you don't need to override fun onResponse manually in every instance.

I think I want to be able to write this:

wrapper.execute(
    SEND_NOTIFICATION_ACTION_TYPE,
    SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),
    object: SomeNewType<ActionResponse, SendNotificationResponse>
}

I was trying to extract a wrapActionListener method though had problem with recreateObject() function which requires reified type (generic type didn't work). Let me give it another try. Thanks!

[dblock]

@joshuali925 excellent!

Is it worth putting this into a method (e.g. executeAction) that takes DELETE_NOTIFICATION_CONFIG_ACTION_TYPE, and DeleteNotificationConfigResponse or are we trying too hard?

client.execute(
            DELETE_NOTIFICATION_CONFIG_ACTION_TYPE,
            request,
            listener
            wrapActionListener(listener) { response -> recreateObject(response) { DeleteNotificationConfigResponse(it) } }
        )

I almost think that maybe this entire implementation is a map of DELETE_NOTIFICATION_CONFIG_ACTION_TYPE -> DeleteNotificationConfigResponse, but it's getting very meta. 🤔

[joshuali925]

@dblock agree that could look cleaner, but in sendNotification the client is wrapped with SecureClientWrapper before calling execute. I assume we'll need to make this an exception if we generalize the execute function, and i'm not sure if there will be other exceptions in the future. Anyways I'll also defer to @dai-chen since he is coming up with the fixes and refactoring

common-utils/src/main/kotlin/org/opensearch/commons/notifications/NotificationsPluginInterface.kt

Lines 218 to 225 in d5bcf10

	val threadContext: String? =
	client.threadPool().threadContext.getTransient<String>(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT)
	val wrapper = SecureClientWrapper(client) // Executing request in privileged mode
	wrapper.execute(
	SEND_NOTIFICATION_ACTION_TYPE,
	SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),
	wrapActionListener(listener) { response -> recreateObject(response) { SendNotificationResponse(it) } }
	)

[dai-chen]

Closing for #40

[dai-chen]

@dblock agree that could look cleaner, but in sendNotification the client is wrapped with SecureClientWrapper before calling execute. I assume we'll need to make this an exception if we generalize the execute function, and i'm not sure if there will be other exceptions in the future. Anyways I'll also defer to @dai-chen since he is coming up with the fixes and refactoring

common-utils/src/main/kotlin/org/opensearch/commons/notifications/NotificationsPluginInterface.kt

Lines 218 to 225 in d5bcf10

	val threadContext: String? =
	client.threadPool().threadContext.getTransient<String>(OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT)
	val wrapper = SecureClientWrapper(client) // Executing request in privileged mode
	wrapper.execute(
	SEND_NOTIFICATION_ACTION_TYPE,
	SendNotificationRequest(eventSource, channelMessage, channelIds, threadContext),
	wrapActionListener(listener) { response -> recreateObject(response) { SendNotificationResponse(it) } }
	)

@joshuali925 @dblock I tried this idea but the code was not clean as I thought. This is mostly because of generic types and number fo params required by each execute call. I would suggest keeping it as is for now.