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

add non-public tweet metrics and misc fields for API v2 #366

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

xberger
Copy link

@xberger xberger commented Mar 23, 2022

Well. There are some design decisions that need to be discussed.

Tweet interface:
Obviously, API v1 and v2 are parting ways now even more. How to deal with that in the Interface Tweet and in the class TweetV1 with the v2 only getters like getImpressionCount()? Now it returns 0. Should it better throw an exception? Return -1? Changing return type to return null?

Non-public:
What to return on non-public metrics in the DTOs, if requested in a public context? This metrics do not exist then and just returning 0 (which is happening) is misleading. To get around it, I added some has-methods (e.g. hasOrganicMetrics()). (For consistency reasons I even added them for public metrics in tweets and user.) For the single metrics I still would prefer returning Integer null instead of int 0.

Error management:
There seems to be a whole world, where twitter returns with http 200, but has only errors in the response. This happens on insufficient access rights for non-public data. Couldn't even find docs on that on twitter. I throw NoPermissionException on a certain condition now, but that is highly preliminarily. Further knowledge is still missing here what can happen.

Consumer key context:
To determine whether requesting with consumer key for user context or app-only bearer token, a var useConsumerKey in RequestHelperV2 is used. But that would mean constructor explosion. So I did not add it to all constructors. I guess that is not final. Furthermore, it maybe would be nice to determine that per request, not per TwitterClient/RequestHelperV2.

Tests:
I skimmed over the tests but did not really had an idea.. I need some suggestions here what to do.

* @param requestTweetFieldsScope
* @return a TweetList object containing a list of tweets and the next token if recursiveCall is set to false
*/
public TweetList getUserTimeline(String userId, AdditionalParameters additionalParameters, REQUEST_TWEET_FIELDS_SCOPE requestTweetFieldsScope);
Copy link
Owner

Choose a reason for hiding this comment

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

Little question, isn't it possible to add the requestTweetFieldsScope parameter inside the additionalParameters object?
It will avoid creating new constructors no ?

@redouane59
Copy link
Owner

redouane59 commented Mar 23, 2022

Hey,

Thank you very much for your time !

Tweet interface:
Obviously, API v1 and v2 are parting ways now even more. How to deal with that in the Interface Tweet and in the class TweetV1 with the v2 only getters like getImpressionCount()? Now it returns 0. Should it better throw an exception? Return -1? Changing return type to return null?

Good question, personally I would throw an exception (and use @JsonIgnore to avoid calling the methods during serialization) but as you are currently the only one using it, you can decide how you want to implement it. Maybe later other users will challenge this choice.

For the other points, it looks relevant.

Finally for the tests, you should create json sample files and try to deserialize them and be sure that all fields are OK (as I did for TweetV2 object for example).

@xberger
Copy link
Author

xberger commented Mar 24, 2022

Hi,

thanks for the feedback. Will work on it. At the moment I'm still trying requests and learn something about the partial API errors.

@gitguardian
Copy link

gitguardian bot commented Nov 8, 2022

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Generic High Entropy Secret 5d49d4a src/test/resources/tests/dm_simple_example_v2.json View secret
- Generic High Entropy Secret 5d49d4a src/test/resources/tests/dm_simple_example_v2.json View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@@ -104,14 +104,12 @@ public class TwitterClient implements ITwitterClientV1, ITwitterClientV2, ITwitt
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
.setSerializationInclusion(JsonInclude.Include.NON_NULL)
.findAndRegisterModules();
public static final String TWEET_FIELDS = "tweet.fields";
Copy link
Owner

Choose a reason for hiding this comment

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

please use the same code style to avoid this

@redouane59
Copy link
Owner

Did you leave it up ? Can I close it?

@xberger
Copy link
Author

xberger commented Nov 9, 2022

Did you leave it up ? Can I close it?

Well.. I ran out of breath for this pull request back in March and just left it as it was in my project. In retrospect it should not have been a pull request at this point, since the error handling I added was erroneous.

Now I want to improve the error handling, so I merged. I think there must be dto objects for mixed state (where you have returned objects and error messages at the same time). Like the class TweetError, but that class is never used, as far as I can see?

So I'm now working on it again, so you can leave it open at the moment.

Well, now non-public data and error handling are intertwined, which is not good. I should maybe revert the error handling to have a clean state for this pull-request. Sorry for the chaos.

@xberger
Copy link
Author

xberger commented Nov 14, 2022

@redouane59 I did another update with some minor changes discusses earlier. Wanted to test it in my project, but the update of com.fasterxml.jackson didn't work out with the rest of my project. Things got incompatible. I think my set up here is a mess. I lack the routine regarding integration. Maybe it will work out later.

@redouane59
Copy link
Owner

Do you have a similar problem than #427 ?

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