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

Replacing Jackson Factory with Gson Factory #32148

Closed
wants to merge 2 commits into from

Conversation

cutiepie-10
Copy link
Contributor

This pull request fixes [#32130].
Tried to remove the bugs in the previous pull request.

@cutiepie-10 cutiepie-10 changed the title Rep[lacing Jackson Factory with Gson Factory Replacing Jackson Factory with Gson Factory Aug 11, 2024
Copy link
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

Copy link
Contributor

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Thanks! This looks pretty good, I had a couple of comments though

@@ -1,3 +1,4 @@
#Sat Aug 10 17:06:24 IST 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this was added by mistake, could you please remove it?

@@ -23,7 +23,7 @@
import com.google.api.client.http.HttpRequestInitializer;
import com.google.api.client.http.HttpTransport;
import com.google.api.client.json.JsonFactory;
import com.google.api.client.json.jackson2.JacksonFactory;
import com.google.api.client.json.gson.GsonFactory;
Copy link
Contributor

Choose a reason for hiding this comment

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

From the precommits (https://github.com/apache/beam/actions/runs/10342154630/job/28624760220?pr=32148), I see the following error:

Execution failed for task ':sdks:java:extensions:google-cloud-platform-core:analyzeClassesDependencies'.
> Dependency analysis found issues.
  usedUndeclaredArtifacts
   - com.google.http-client:google-http-client-gson:1.44.1@jar
  unusedDeclaredArtifacts
   - com.google.http-client:google-http-client-jackson2:1.44.1@jar

I think we need to replace the jackson artifact with the gson one here -

implementation library.java.google_http_client_jackson2

(example where we've already included the gson library -

implementation library.java.jackson_core
)

@liferoad
Copy link
Collaborator

@damccorm
Copy link
Contributor

Hey @cutiepie-10 we can keep this PR open, it just needs a few minor changes!

@damccorm damccorm reopened this Aug 12, 2024
@cutiepie-10
Copy link
Contributor Author

Hi @damccorm ,
Please checkout the new pull request on #32130. It has the latest changes. Actually by mistake, I made commit on the other branch and this branch could not detect new changes. It is my request to check the other branch.

@damccorm
Copy link
Contributor

No problem

@damccorm damccorm closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants