7247: Alternate for Twitter4J to support twitter plug-in #323

Suchitainf · 2021-10-16T13:13:32Z

This PR addresses the issue of using Twitter4J as third party library for JMC, which is not actively maintained by the owner of Twitter4J. The last release was 3 years back. We have tried contacting the owner for the next release dates but couldn't get a proper response. Hence, we would need to remove the dependency from JMC because we should not use any dependency which is not well maintained.

JMC Console gives the functionality of setting up triggers for particular conditions which can be set in configuration. Twitter is used as one of the trigger methods to notify users (or tweeters in case of Twitter). I have removed the Twitter4J dependency completely and using all the twitter related APIs directly. There are mainly three parts of this PR (functionality for which Twitter4J was used).

Authentication : The authentication of twitter users i.e. tweeters are done as part of preference settings. Twitter follows 3-legged OAuth authentication process for authenticating any user. So there are 3 steps which I have implemented in order to authenticate any user and save his details in preferences. Once the tweeter is verified the JMC application will use his details for verification before any action is triggered for that user. In order to use any user name we need to authorize his/her credentials in JMC preference.
Send Direct Message : A verified user (tweeter) can send a direct message to another user (tweeter). The first user should be verified user i.e. it should be one of the users listed in preference. The second user need not be a part of authorized users list but before sending any message we are checking whether the user exists or not.
Update Status : A JMC user can update the status of any verified user (tweeter) on Twitter which can be seen on twitter wall of that user.

I have used http classes of JDK 11 for making REST API calls, so this plugin will be dependent on JDK 11.

There is a scope of using a JSON library to make the code better but as of now I haven't used any third party library for this implementation may be we can modify the code in future if we have some JSON library dependency in future.

Please review the same and let me know your valuable comments (if any). Also, feel free to ping me if you want to test it and stuck somewhere. I can help with the steps to test it.

Progress

Commit message must refer to an issue
Change must be properly reviewed

Issue

JMC-7247: Alternate for Twitter4J to support twitter plug-in

Reviewers

Alex Macdonald (@aptmac - Committer) ⚠️ Review applies to a89db1c
Marcus Hirt (@thegreystone - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jmc pull/323/head:pull/323
$ git checkout pull/323

Update a local copy of the PR:
$ git checkout pull/323
$ git pull https://git.openjdk.java.net/jmc pull/323/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 323

View PR using the GUI difftool:
$ git pr show -t 323

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jmc/pull/323.diff

bridgekeeper · 2021-10-16T13:14:16Z

👋 Welcome back schaturvedi! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2021-10-16T13:19:05Z

Webrevs

aptmac · 2021-10-20T20:38:02Z

Neat! I've tried giving this a try, but I've run into a problem when trying to send a tweet. I've set up my Twitter developer account, and managed to add my account into JMC.

But when I set up Twitter as an action, nothing happens when the trigger criteria is met.

(EDIT: the screenshot below makes it look like I'm using an incorrect username, but the underscore is getting hidden by the bottom of the textbox, the text really is "aptmac_" even though it looks like it's missing the underscore)

Looking at the console, there looks to be an exception thrown. I'm assuming that the id number is specific to my account, so I've replaced it with "123456789", but here's the error below. It looks like there's a trailing comma that's causing the parsing of the number to crash. From a quick look at the logs, it looks like the sub-string parsing in TwitterPlugin for the user id is off: https://github.com/openjdk/jmc/pull/323/files#diff-7462129c16153b6d0d6ac4d57c25d5cf1bc58793a688c87552752e3f0866475dR258

Oct. 20, 2021 4:14:18 P.M. org.openjdk.jmc.rjmx.triggers.internal.DefaultExceptionHandler handleException
SEVERE: Could not invoke the action for the rule CPU Usage - JVM Process (Too High)
java.lang.NumberFormatException: For input string: "123456789,"id_str":"
	at java.base/java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
	at java.base/java.lang.Long.parseLong(Long.java:692)
	at java.base/java.lang.Long.valueOf(Long.java:1144)
	at org.openjdk.jmc.console.twitter.TwitterPlugin.getUserId(TwitterPlugin.java:259)
	at org.openjdk.jmc.console.twitter.TwitterPlugin.sendDirectMessage(TwitterPlugin.java:381)
	at org.openjdk.jmc.console.twitter.SendMessage.send(SendMessage.java:64)
	at org.openjdk.jmc.console.twitter.SendMessage.handleNotificationEvent(SendMessage.java:59)
	at org.openjdk.jmc.rjmx.triggers.internal.NotificationTrigger.doTrigger(NotificationTrigger.java:329)
	at org.openjdk.jmc.rjmx.triggers.internal.NotificationTrigger.triggerOn(NotificationTrigger.java:264)
	at org.openjdk.jmc.rjmx.triggers.internal.NotificationRuleBag$1.valueChanged(NotificationRuleBag.java:106)
	at org.openjdk.jmc.rjmx.subscription.internal.AbstractAttributeSubscription.fireAttributeChange(AbstractAttributeSubscription.java:161)
	at org.openjdk.jmc.rjmx.subscription.internal.AbstractAttributeSubscription.storeAndFireEvent(AbstractAttributeSubscription.java:192)
	at org.openjdk.jmc.rjmx.subscription.internal.DefaultAttributeSubscriptionThread.dispatchEvents(DefaultAttributeSubscriptionThread.java:440)
	at org.openjdk.jmc.rjmx.subscription.internal.DefaultAttributeSubscriptionThread.retrieveAndDispatchNormalAttributes(DefaultAttributeSubscriptionThread.java:159)
	at org.openjdk.jmc.rjmx.subscription.internal.DefaultAttributeSubscriptionThread.retrieveAndDispatchValues(DefaultAttributeSubscriptionThread.java:150)
	at org.openjdk.jmc.rjmx.subscription.internal.DefaultAttributeSubscriptionThread.run(DefaultAttributeSubscriptionThread.java:117)

Aside from that, when linking my Twitter account in the JMC preferences, the headers for Consumer Key and Consumer Secret have since been updated on the Twitter side to be "API Key" and "API Key Secret", the JMC preferences page should probably get updated as well.

I'll start poking around the code soon, but for the time being I wasn't able to send Tweets.

This PR addresses the issue of using Twitter4J as third party library for JMC, which is not actively maintained by the owner of Twitter4J. The last release was 3 years back. We have tried contacting the owner for the next release dates but couldn't get a proper response. Hence, we would need to remove the dependency from JMC because we should not use any dependency which is not well maintained.

Out of curiosity, is there a specific time frame that dependencies should be updated for JMC? It looks like Twitter4J had a commit merged earlier this year, but you're right that it hasn't had major functionality updates in the last while. For comparison, the last Twitter4J release was 1,159 days ago, but in terms of dependencies we consume our jaf 1.2.1 is 1,059 days old, our Jemmy 2.0.0 is 867 days old, HdrHistogram 2.1.12 is 680 days old, etc.

I have used http classes of JDK 11 for making REST API calls, so this plugin will be dependent on JDK 11.

That should be okay for now, after the Eclipse platform update last year we need to use JDK 11 as a minimum anyways.

Suchitainf · 2021-10-20T20:47:49Z

@aptmac Thanks for taking out time for review. Can you please double check whether you have used your correct username while triggering the update status flow? I can see the underscore missing at the end. Please correct me if I am wrong.

aptmac · 2021-10-20T21:01:45Z

Yeah, my Twitter username is "aptmac_", and the app authenticated okay with JMC.

Suchitainf · 2021-10-20T21:05:35Z

But the update status screenshot doesn't show underscore at the end. Also, the screenshot shared is of Update Status and the logs which you shared is of Send Direct Message flow. The update status flow will need only one username which should be "aptmac_" in your case and for send direct message there should be two usernames. From should be "aptmac_" and to should be some other valid user name. I have tested by giving my username in both from and to.

aptmac · 2021-10-20T21:19:35Z

But the update status screenshot doesn't show underscore at the end.

I typed it in there, but the underscore gets hidden by the bottom of the textbox, the screenshot is a bit misleading but the text in there is "aptmac_"

Also, the screenshot shared is of Update Status and the logs which you shared is of Send Direct Message flow. The update status flow will need only one username which should be "aptmac_" in your case and for send direct message there should be two usernames. From should be "aptmac_" and to should be some other valid user name. I have tested by giving my username in both from and to.

Yeah, I tried both ways, but the exception was thrown when I first tried to Direct Message, and there I was trying to send a DM from myself to myself (I can in the Twitter UI, so thought maybe I could do the same here). Then I went to try to broadcast a message there wasn't anything happening.

Suchitainf · 2021-10-20T21:22:50Z

Ok I understood the issue. Even I am not able to send message from my twitter to yours. I will fix the issue and raise it for review again.

aptmac · 2021-10-27T16:21:35Z

Nice! It's working much better now, I verified the status and messaging functionality:
https://twitter.com/aptmac_/status/1453395400669401093

Something I did notice is that it would be nice if an error dialog could show up in the event that a Tweet cannot be sent. For example, I set up my keys and account but had my Twitter application defaulted to read-only. JMC was rightfully getting a 401 trying to POST, but there wasn't indication of anything not working in the JMC ui, so it took me a bit of time to debug. I also found out that it's not enough to just set the Twitter application settings to write+message, I had to re-attach my keys in JMC before it was able to send a 200 response properly.

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java

Suchitainf · 2021-11-09T21:18:01Z

I have enhanced the error handling for both the flows. I have tested it by creating a new twitter app which is having only read-only access and in preference added that app for my twitter username. When I tried triggering update status and send direct message I am getting below error messages.

aptmac

It looks good! I added a couple comments mentioning that it'd be nice to include the error code along with the dialog.

Also, there are quite a few strings that could be externalized, it would be nice to pull them into a Messages class

aptmac · 2021-11-10T16:24:25Z

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java

+		HttpResponse<String> response = getHttpClient().send(request, HttpResponse.BodyHandlers.ofString());
+		if (response.statusCode() != 200) {
+			JOptionPane.showMessageDialog(null,
+					" Some error occured while sending direct message. Please verify your twitter app settings. ");


There are trailing spaces at the beginning and end of this string. It would also be nice to have the error status included with this string, so the user has a better idea of where an issue might have happened if it occurs. The string in the dialog might be something like:

"An error occured while __________. Please verify your Twitter app settings. (Error code: XXX)"

aptmac · 2021-11-10T16:27:15Z

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java

+		HttpResponse<String> response = getHttpClient().send(request, HttpResponse.BodyHandlers.ofString());
+		if (response.statusCode() != 200) {
+			JOptionPane.showMessageDialog(null,
+					" Some error occured while updating Twitter status. Please verify your twitter app settings. ");


Similar to the string usage below, it'd be nice to include the error code along with this message.

Suchitainf · 2021-11-10T21:34:41Z

The new error message screen looks like this:

aptmac

The updated error message looks great, thanks.

I've commented on a handful of lines that could also be externalized.

aptmac · 2021-11-15T19:36:58Z

...onsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthAunthenticator.java

+
+			return oauth_token;
+		} catch (Exception e) {
+			LOGGER.log(Level.SEVERE, "Unable to get request token", e);


This string could be placed in Messages.java

aptmac · 2021-11-15T19:37:16Z

...onsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthAunthenticator.java

+
+			return accessToken;
+		} catch (Exception e) {
+			LOGGER.log(Level.SEVERE, "Unable to authenticate.", e);


This string could be placed in Messages.java

aptmac · 2021-11-15T19:37:27Z

...onsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthAunthenticator.java

+			}
+		} catch (URISyntaxException e) {
+			// Should never happen...
+			LOGGER.log(Level.SEVERE, "Failed to parse URI", e);


This string could be placed in Messages.java

aptmac · 2021-11-15T19:39:01Z

...onsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthAunthenticator.java

+		SecretKey secretKey = null;
+
+		byte[] keyBytes = keyString.getBytes();
+		secretKey = new SecretKeySpec(keyBytes, "HmacSHA1");


Perhaps the "HmacSHA1" could be a global variable instead of a quoted string here (as it's used again below)?

aptmac · 2021-11-15T19:39:26Z

...nsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthHeaderGenerator.java

+			mac = Mac.getInstance(HMAC_SHA1);
+			mac.init(key);
+		} catch (NoSuchAlgorithmException | InvalidKeyException e) {
+			LOGGER.log(Level.SEVERE, "Failed to encrypt.", e);


This string could be placed in Messages.java

aptmac · 2021-11-15T19:39:32Z

...nsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthHeaderGenerator.java

+		try {
+			encoded = Encode.forUriComponent(value);
+		} catch (Exception e) {
+			LOGGER.log(Level.SEVERE, "Failed to encode the URL.", e);


This string could be placed in Messages.java

aptmac · 2021-11-15T19:39:57Z

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java

+			}
+		} catch (URISyntaxException e) {
+			// Should never happen...
+			LOGGER.log(Level.SEVERE, "Failed to parse URI", e);


This string could be placed in Messages.java

aptmac · 2021-11-15T19:40:19Z

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java

+					}
+					storeAndSavePrefs();
+				} else {
+					DialogToolkit.showError(null, "Invalid User", "The authorized user is invalid.");


This string could be placed in Messages.java

openjdk · 2021-11-17T20:46:02Z

@Suchitainf This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

7247: Alternate for Twitter4J to support twitter plug-in

Reviewed-by: aptmac, hirt

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 12 new commits pushed to the master branch:

3ce6483: 7447: Add serializers and writer modules to core test coverage
35c8e0e: 7446: Add Maven profile for enabling test coverage
24f4cb0: 5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors
d218205: 7440: JMC fails on Apple M1
77580fe: 7418: Stop using Collections sort method
ef36b79: 7437: Fix require bundle error in Eclipse
1d17e0c: 7435: Update the maven enforcer plugin
cf4ceb3: 7434: Update the maven download plugin
9bb5799: 7433: Update the spotless maven plugin
5787b9a: 7091: JOverflow Instance View table should be sortable by clicking headers
... and 2 more: https://git.openjdk.java.net/jmc/compare/3424e723dba8cc0196697a9c2b0e771d30c17ac6...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

thegreystone · 2021-11-24T15:38:06Z

Since we are removing the dependency on Twitter4J, we should also remove it from https://github.com/openjdk/jmc/blob/master/license/THIRDPARTYREADME.txt#L265.

...onsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthAunthenticator.java

...nsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthHeaderGenerator.java

thegreystone · 2021-11-25T19:30:58Z

/reviewers 2

openjdk · 2021-11-25T19:31:43Z

@thegreystone
The number of required reviews for this PR is now set to 2 (with at least 1 of role committers).

Suchitainf · 2021-11-28T19:50:50Z

/integrate

openjdk · 2021-11-28T19:51:27Z

Going to push as commit 4d1c087.
Since your change was applied there have been 12 commits pushed to the master branch:

3ce6483: 7447: Add serializers and writer modules to core test coverage
35c8e0e: 7446: Add Maven profile for enabling test coverage
24f4cb0: 5904: Merge org.openjdk.jmc.ui.celleditors with org.openjdk.jmc.rjmx.ui.celleditors
d218205: 7440: JMC fails on Apple M1
77580fe: 7418: Stop using Collections sort method
ef36b79: 7437: Fix require bundle error in Eclipse
1d17e0c: 7435: Update the maven enforcer plugin
cf4ceb3: 7434: Update the maven download plugin
9bb5799: 7433: Update the spotless maven plugin
5787b9a: 7091: JOverflow Instance View table should be sortable by clicking headers
... and 2 more: https://git.openjdk.java.net/jmc/compare/3424e723dba8cc0196697a9c2b0e771d30c17ac6...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-28T19:51:49Z

@Suchitainf Pushed as commit 4d1c087.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

7247: Alternate for Twitter4J to support twitter plug-in

e2f22de

openjdk bot added the rfr label Oct 16, 2021

thegreystone added the hacktoberfest-accepted label Oct 22, 2021

Fixed an issue in direct message flow for user id.

da3b53c

aptmac reviewed Oct 27, 2021

View reviewed changes

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java Outdated Show resolved Hide resolved

...openjdk.jmc.console.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterPlugin.java Outdated Show resolved Hide resolved

Enhanced error handling

92c3e07

aptmac reviewed Nov 10, 2021

View reviewed changes

Moved all error messages to properties file and added error code

c769e32

aptmac reviewed Nov 15, 2021

View reviewed changes

Moved all the hard coded strings to generic Messages

a89db1c

aptmac approved these changes Nov 17, 2021

View reviewed changes

openjdk bot added the ready label Nov 17, 2021

Removed third party dependency of Twitter4J from license file

33fb63d

thegreystone requested changes Nov 24, 2021

View reviewed changes

...onsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthAunthenticator.java Outdated Show resolved Hide resolved

...nsole.twitter/src/main/java/org/openjdk/jmc/console/twitter/TwitterOAuthHeaderGenerator.java Outdated Show resolved Hide resolved

openjdk bot removed the ready label Nov 25, 2021

Implemented some review comments

f2b0a35

thegreystone approved these changes Nov 27, 2021

View reviewed changes

openjdk bot added the ready label Nov 27, 2021

openjdk bot closed this Nov 28, 2021

openjdk bot added integrated and removed ready rfr labels Nov 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7247: Alternate for Twitter4J to support twitter plug-in #323

7247: Alternate for Twitter4J to support twitter plug-in #323

Suchitainf commented Oct 16, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 16, 2021

mlbridge bot commented Oct 16, 2021 •

edited

Loading

aptmac commented Oct 20, 2021 •

edited

Loading

Suchitainf commented Oct 20, 2021

aptmac commented Oct 20, 2021

Suchitainf commented Oct 20, 2021

aptmac commented Oct 20, 2021 •

edited

Loading

Suchitainf commented Oct 20, 2021

aptmac commented Oct 27, 2021

Suchitainf commented Nov 9, 2021

aptmac left a comment

aptmac Nov 10, 2021

aptmac Nov 10, 2021

Suchitainf commented Nov 10, 2021

aptmac left a comment

aptmac Nov 15, 2021

aptmac Nov 15, 2021

aptmac Nov 15, 2021

aptmac Nov 15, 2021

aptmac Nov 15, 2021

aptmac Nov 15, 2021

aptmac Nov 15, 2021

aptmac Nov 15, 2021

openjdk bot commented Nov 17, 2021 •

edited

Loading

thegreystone commented Nov 24, 2021

thegreystone commented Nov 25, 2021

openjdk bot commented Nov 25, 2021

Suchitainf commented Nov 28, 2021

openjdk bot commented Nov 28, 2021

openjdk bot commented Nov 28, 2021

7247: Alternate for Twitter4J to support twitter plug-in #323

7247: Alternate for Twitter4J to support twitter plug-in #323

Conversation

Suchitainf commented Oct 16, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 16, 2021

mlbridge bot commented Oct 16, 2021 • edited Loading

Webrevs

aptmac commented Oct 20, 2021 • edited Loading

Suchitainf commented Oct 20, 2021

aptmac commented Oct 20, 2021

Suchitainf commented Oct 20, 2021

aptmac commented Oct 20, 2021 • edited Loading

Suchitainf commented Oct 20, 2021

aptmac commented Oct 27, 2021

Suchitainf commented Nov 9, 2021

aptmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suchitainf commented Nov 10, 2021

aptmac left a 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

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

openjdk bot commented Nov 17, 2021 • edited Loading

thegreystone commented Nov 24, 2021

thegreystone commented Nov 25, 2021

openjdk bot commented Nov 25, 2021

Suchitainf commented Nov 28, 2021

openjdk bot commented Nov 28, 2021

openjdk bot commented Nov 28, 2021

Suchitainf commented Oct 16, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 16, 2021 •

edited

Loading

aptmac commented Oct 20, 2021 •

edited

Loading

aptmac commented Oct 20, 2021 •

edited

Loading

openjdk bot commented Nov 17, 2021 •

edited

Loading