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

Fix #3406: change default of DeserializationFeature.FAIL_ON_TRAILING_TOKENS to true #4902

Merged
merged 15 commits into from
Jan 30, 2025

Conversation

cowtowncoder
Copy link
Member

(see #3406 for details)

@cowtowncoder
Copy link
Member Author

As per #3406: 11 test failures (for both 2.19 and 3.0), all for same root cause likely.
A seemingly real issue, only exposed with changed settings. Scary; and also something that may block this change altogether. No luck figuring out how to fix, as of yet.

@JooHyukKim
Copy link
Member

@cowtowncoder I tried to attempt fixing the remaining tests but couldn't compile the master branch in IntelliJ due to issues with (I suppose) module-info.java. Even after clearing the cache, no luck. Have you faced this locally?

Symptoms are tests can't import ObjectMapper... 🥲

@cowtowncoder
Copy link
Member Author

@JooHyukKim I have some issues with Eclipse, but build/test always works from command-line Maven. I wonder if I just need to give up on current reusable test jar idea, create separate artifact.

@JooHyukKim
Copy link
Member

@JooHyukKim I have some issues with Eclipse, but build/test always works from command-line Maven. I wonder if I just need to give up on current reusable test jar idea, create separate artifact.

We don't have that much to change at this point. Maybe we can come back to this reusability idea when we need to change things alot? 🤔

@cowtowncoder
Copy link
Member Author

@JooHyukKim I have some issues with Eclipse, but build/test always works from command-line Maven. I wonder if I just need to give up on current reusable test jar idea, create separate artifact.

We don't have that much to change at this point. Maybe we can come back to this reusability idea when we need to change things alot? 🤔

It's just such a pity since if this worked it'd be very easy -- everything is all in and with minimal additional maintenance. Now we have to duplicate those 3 annotation classes everywhere so if anything needs to change we have a dozen repos to change.

@JooHyukKim
Copy link
Member

@cowtowncoder Seems like we are good to go.

After looking at the remaining test failures and what the tests trying to do, seems like we are only left with irrelevant JSON sytnax errors. Filed PR #4941 that extended this PR for your reference

@cowtowncoder cowtowncoder merged commit 58ca912 into master Jan 30, 2025
6 checks passed
@cowtowncoder cowtowncoder deleted the tatu/3.0/3406-FAIL_ON_TRAILING-defaults branch January 30, 2025 01:28
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