Skip to content
This repository was archived by the owner on Sep 12, 2024. It is now read-only.

Support jackson 2.11.0+ - #1301

Open
jwiklund wants to merge 1 commit into
spotify:masterfrom
jwiklund:jackson-2-11
Open

Support jackson 2.11.0+#1301
jwiklund wants to merge 1 commit into
spotify:masterfrom
jwiklund:jackson-2-11

Conversation

@jwiklund

@jwiklund jwiklund commented Aug 6, 2020

Copy link
Copy Markdown

Jackson 2.11.0 changes the default _tzSerializedWithColon value from false to true,
this causes a "Id hash mismatch" when trying to deploy a job.
withColonInTimeZone has been available since 2.9.1 so this will break on older version,
If this is an issue we would probably have to resort to reflection.

Jackson 2.11.0 changes the default _tzSerializedWithColon value from false to true.
This causes a "Id hash mismatch" when trying to deploy a job.

withColonInTimeZone has been available since 2.9.1 so this will break on older version.
If this is an issue we would probably have to resort to reflection.
@jwiklund
jwiklund requested a review from a team as a code owner August 6, 2020 13:15
@codecov

codecov Bot commented Aug 6, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1301 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1301      +/-   ##
============================================
+ Coverage     53.02%   53.04%   +0.01%     
- Complexity     1657     1658       +1     
============================================
  Files           269      269              
  Lines         13135    13138       +3     
  Branches       1505     1505              
============================================
+ Hits           6965     6969       +4     
+ Misses         5656     5655       -1     
  Partials        514      514              

@davidxia

davidxia commented Aug 6, 2020

Copy link
Copy Markdown
Contributor

Just curious what you need newer jackson versions for. Is it to fix this vuln? https://github.com/spotify/helios/network/alert/pom.xml/com.fasterxml.jackson.core:jackson-databind/open

@jwiklund

jwiklund commented Aug 6, 2020

Copy link
Copy Markdown
Author

@davidxia We have a mixed repository with both apollo and scio (to test the combination) and the newest scio 0.9.2 requires jackson 2.11.0.

@davidxia

davidxia commented Aug 6, 2020

Copy link
Copy Markdown
Contributor

Are you able to do some shading for just your service? Helios is in maintenance mode and rolling this out will affect the entire fleet and take a while.

@jwiklund

jwiklund commented Aug 6, 2020

Copy link
Copy Markdown
Author

I have copied this Json file into our repository which seems to fix the problem for us. I guess we can keep this open if someone else encounters the same problem to avoid pushing this out.

@davidxia

davidxia commented Aug 6, 2020

Copy link
Copy Markdown
Contributor

oh nice, which JSON file are you referring to?

@jwiklund

jwiklund commented Aug 6, 2020

Copy link
Copy Markdown
Author

I mean the Json.java file in this repository, I applied this PR's change and have it in our repository and it loads before the Json class from helios-client.jar :)

@jwiklund

jwiklund commented Aug 6, 2020

Copy link
Copy Markdown
Author

Since it's only used for testing it's kind of okay :)

@davidxia

davidxia commented Aug 6, 2020

Copy link
Copy Markdown
Contributor

Whoa, what dark magic is this?

@jwiklund

jwiklund commented Aug 6, 2020

Copy link
Copy Markdown
Author

Well pre modules everything is a flat namespace so java will load the first file that matches the name (and local files are placed before jar files by maven), I'm assuming if we turn on module support this way of hacking stuff will stop working but it's convenient when needed (and very annoying when it's done by misstake and you have to figure out why that class you can clearly see have a function does not).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants