Parse authParamsString on plugin side #721

tkb77 · 2017-08-29T03:55:29Z

Motivation

#671

Parse authParamsString on plugin side

Modifications

I added EncodedAuthenticationParameterSupport interface to parse authParamsString on plugin side.

If EncodedAuthenticationParameterSupport is implemented, parse authParamsString on plugin side.
If EncodedAuthenticationParameterSupport is not implemented, use default parsing logic(separated by “:” and “,”) on AuthenticationFactory.

Result

We can parse authParamsString on plugins side by overriding configure(String authParamsString) in EncodedAuthenticationParameterSupport interface.

merlimat · 2017-08-30T00:15:35Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/Authentication.java

+     */
+    default void configure(String authParams) {
+        Map<String, String> authParamsMap = new HashMap<>();
+        Logger LOG = LoggerFactory.getLogger(Authentication.class);


logger should be declared as private static final at the class scope

private static final can not be declared at the class scope because Authentication.java is an interface.
https://stackoverflow.com/questions/31452118/why-private-static-field-is-not-allowed-in-java-8-interface

Ok, though instead of having the parsing logic here in the interface, we could have it in a different place and then call it. eg:

default void configure(String authParamsStr) { Map<String, String> authParams = AuthenticationUtil.configureFromJsonString(authParamsStr); configure(authParams); }

Do we really want to have the two lines here? If we move parsing logic to somewhere else, we can deprecate the old interface and will have this only one universal configure interface eventually, which is more beautiful API I think.

I guess the intention for using default method was to avoid breaking the Authentication API. I think we could as well break it right now, since I'm not aware of any 3rd party Authentication implementations at this point.

I guess so, but probably we can achieve the both, keeping compatibility and having one configure (and deprecated old one) if we introduce an interface I wrote on the issue.

merlimat · 2017-08-30T00:16:09Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/Authentication.java

+                authParamsMap = jsonMapper.readValue(authParams, new TypeReference<HashMap<String, String>>() {});
+            } catch (JsonParseException e) {
+                // If authParams is not JSON, separate by “:” and “,” for compatibility
+                LOG.warn("Authentication config parameter should be JSON: {}", e.getMessage());


If we're going to support both comma separated and Json, we shouldn't print a warning here

I think that comma separated should be deleted in the future.
What do you think that?

In any case the deprecation should be at the interface level, not by having WARN message in log.

rdhabalia · 2017-08-30T22:40:09Z

pulsar-client/src/main/java/org/apache/pulsar/client/api/AuthenticationFactory.java

+            if (isNotBlank(authPluginClassName)) {
+                Class<?> authClass = Class.forName(authPluginClassName);
+                Authentication auth = (Authentication) authClass.newInstance();
+                auth.configure(authParamsString);


I feel configure(Map<String, String> authParams) is generic-enough and parsing/preparation of authParams-map can be done by AuthenticationFactory because in future, if we decide yaml format then we can adapt it without changing interface-definition.

a quick thought on implementation:
Rather adding void configure(String authParams) method in Authentication.java and doing parsing, instead can we do parsing here in AuthenticationFactory?

First try to parse given auth-param into json and prepare Map<String,String> from json and call configure(map)
Map<String, String> authParamsMap = jsonMapper.readValue(authParamsString, new TypeReference<HashMap<String, String>>() {});

If fails then try to parse in existing way with : and ; delimiters, and prepare config-map

any thoughts?

I think rdhabalia's suggestion is good enough since what we want to do here is to allow String containing ":" or "," (such as URL) as a parameter.

Introducing new interface sounds nice, however, I think we should do that when we really need plugin-specific parsing.

Well, the new interface may be YAGNI. However, I still don't understand why you guys want limitations.

Maybe Map<String, String> is enough, maybe not. Who knows?

colon-comma, JSON, YAML... What's next? Can we auto-detect all formats?

When we finally agreed on having new interface, is it still easy to migrate? How many plugins exists?

What will we lose instead of freedom if we introduce the new one now?

In my opinion, we should maximize API capability and do smart things on top of it.

What will we lose instead of freedom if we introduce the new one now?

Sure, I will be fine with new api as well. 👍

tkb77 · 2017-09-04T09:28:46Z

I added AuthParamsStringConfigurable interface to parse authParamsString on plugin side.

If AuthParamsStringConfigurable is implemented, parse authParamsString on plugin side.
If authParamsString is JSON, parse JSON on AuthenticationFactory.
If authParamsString is not JSON, use current parsing logic(separated by “:” and “,”) on AuthenticationFactory.

maskit · 2017-09-04T14:07:39Z

@tkb77 Can you explain why we should parse JSON in AuthenticationFactory in spite of the fact that plugins can easily do that in the new configure? It would be just a line.

Map<String, String> authParams = AuthenticationUtil.configureFromJsonString(authParamsStr);

I don't see pros keeping the old configure interface and supporting new format in it. I think it should be left just for compatibility.

Also, format auto-detecting is not so easy.

"abc":"x,y,z"}

Note that { is missing. This string would be parsed as a colon-comma format string, which means that abc would not be set without any errors. This is unsafe.

tkb77 · 2017-09-05T07:17:15Z

@maskit
I can not explain the clear reason.
I try to parse authParamsString on plugin side without changing current parsing logic.
I will only add AuthParamsStringConfigurable interface and enable AuthenticationFactory to use it in this pull request.
I will implement original parsing logic on the plugin side at the next pull request.

tkb77 · 2017-09-07T02:16:41Z

retest this please

nkurihar · 2017-09-07T04:42:19Z

LGTM

maskit

Outline looks good.

AuthParamsStringConfigurable -- The name might be unclear.
Old configure -- Should be deprecated
New configure -- Future plan should be explained on Javadoc

I don't think they are blockers but would be better to change at this time.

maskit · 2017-09-07T13:08:00Z

If I understand you all correctly, our consensus here is:

Replace configure(Map<String, String> with configure(String)
Provide a separated interface to manage compatibility
Add support for JSON to Athens plugin (or provide an utility class and use it from plugins)

and I think we should:

Deprecate the old configure in 1.20 (or 1.21), and remove it in 2.0
Integrate the new configure into Authentication and deprecate the separated interface in 2.0

@tkb77 @merlimat @rdhabalia Makes sense?

tkb77 · 2017-09-08T07:56:21Z

@ maskit
Thank you for your comments.

AuthParamsStringConfigurable -- The name might be unclear.

What did you feel unclear about the name?
Please tell me if there is an idea or suggestion.

Old configure -- Should be deprecated

Does the old interface need to be deprecated?
When deleting the old interface, it is inconvenient because we have to give all settings as a string.

New configure -- Future plan should be explained on Javadoc

I will add javadoc for the new interface.

maskit · 2017-09-08T11:18:11Z

@tkb77

What did you feel unclear about the name?
Please tell me if there is an idea or suggestion.

I can understand the name because we have been talking about it but I'm not confident with that plugin developers easily understand what it is for. The name you suggested doesn't add any context (the same thing is already written as the method name and the parameter name). As I wrote on the issue, I'd suggest EncodedAuthenticationParameterSupport. I'm also fine with Pulsar2AuthenticationInterface if we agreed on marking the old one deprecated.

Does the old interface need to be deprecated?
When deleting the old interface, it is inconvenient because we have to give all settings as a string.

Yes, I think it need to be deprecated because it makes API simple and solid, and it indicates that we are not going to add support for other formats on configure. As I wrote above, plugin developers can parse the string with just one line if we provide an utility class.

Honestly, I'd not write configuration values directly in authParams if I wrote a plugin which requires many and/or complex configurations. I'd write just an external filename or an URL instead. Even if the format was JSON, one line JSON is hard to read / write.

tkb77 · 2017-09-11T08:14:31Z

@maskit
Thank you for advising me.
I fixed as follows.

Add Javadoc
Set deprecated to the old configure
Rename the new interface EncodedAuthenticationParameterSupport

merlimat · 2017-09-12T16:47:16Z

LGTM. @maskit can you do a final check on this?

maskit

LGTM

merlimat

👍

merlimat reviewed Aug 30, 2017

View reviewed changes

rdhabalia reviewed Aug 30, 2017

View reviewed changes

tkb77 force-pushed the parse_authParamsString_on_plugin_side branch from ed1ef85 to 8aa7e91 Compare September 4, 2017 09:27

tkb77 force-pushed the parse_authParamsString_on_plugin_side branch from 8aa7e91 to c633e60 Compare September 5, 2017 07:15

nkurihar approved these changes Sep 7, 2017

View reviewed changes

maskit reviewed Sep 7, 2017

View reviewed changes

Parse authParamsString on plugin side (apache#671)

48831a8

tkb77 force-pushed the parse_authParamsString_on_plugin_side branch from c633e60 to 48831a8 Compare September 11, 2017 08:14

merlimat assigned tkb77 Sep 14, 2017

merlimat added area/client type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Sep 14, 2017

merlimat added this to the 1.20.0-incubating milestone Sep 14, 2017

maskit approved these changes Sep 14, 2017

View reviewed changes

merlimat approved these changes Sep 14, 2017

View reviewed changes

merlimat merged commit 2a04683 into apache:master Sep 14, 2017

yush1ga mentioned this pull request Sep 26, 2017

Modify AuthFactory for C++ Client to enable each plugin to parse authPramsString #792

Merged

sijie mentioned this pull request Apr 25, 2018

Should authParamsString for Authentication support another format ? #671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse authParamsString on plugin side #721

Parse authParamsString on plugin side #721

tkb77 commented Aug 29, 2017 •

edited

Loading

merlimat Aug 30, 2017

tkb77 Aug 30, 2017

merlimat Aug 30, 2017

maskit Aug 30, 2017

merlimat Aug 30, 2017 •

edited

Loading

maskit Aug 30, 2017

merlimat Aug 30, 2017

tkb77 Aug 30, 2017

merlimat Aug 30, 2017

rdhabalia Aug 30, 2017 •

edited

Loading

nkurihar Aug 31, 2017

maskit Aug 31, 2017

rdhabalia Aug 31, 2017

tkb77 commented Sep 4, 2017

maskit commented Sep 4, 2017

tkb77 commented Sep 5, 2017 •

edited

Loading

tkb77 commented Sep 7, 2017

nkurihar commented Sep 7, 2017

maskit left a comment

maskit commented Sep 7, 2017

tkb77 commented Sep 8, 2017 •

edited

Loading

maskit commented Sep 8, 2017

tkb77 commented Sep 11, 2017 •

edited

Loading

merlimat commented Sep 12, 2017

maskit left a comment

merlimat left a comment

Parse authParamsString on plugin side #721

Parse authParamsString on plugin side #721

Conversation

tkb77 commented Aug 29, 2017 • edited Loading

Motivation

Modifications

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merlimat Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdhabalia Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkb77 commented Sep 4, 2017

maskit commented Sep 4, 2017

tkb77 commented Sep 5, 2017 • edited Loading

tkb77 commented Sep 7, 2017

nkurihar commented Sep 7, 2017

maskit left a comment

Choose a reason for hiding this comment

maskit commented Sep 7, 2017

tkb77 commented Sep 8, 2017 • edited Loading

maskit commented Sep 8, 2017

tkb77 commented Sep 11, 2017 • edited Loading

merlimat commented Sep 12, 2017

maskit left a comment

Choose a reason for hiding this comment

merlimat left a comment

Choose a reason for hiding this comment

tkb77 commented Aug 29, 2017 •

edited

Loading

merlimat Aug 30, 2017 •

edited

Loading

rdhabalia Aug 30, 2017 •

edited

Loading

tkb77 commented Sep 5, 2017 •

edited

Loading

tkb77 commented Sep 8, 2017 •

edited

Loading

tkb77 commented Sep 11, 2017 •

edited

Loading