-
Notifications
You must be signed in to change notification settings - Fork 259
-
Notifications
You must be signed in to change notification settings - Fork 259
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
StackOverflow for JsonBodyFilter #689
Comments
This is proably the same proablem as the one reported in #686 |
I think it failed on rc6 as well tho |
@PascalSchumacher It's related, yes. But the issue with long was fixed (my test uses a property with 10,000 characters or something like that. I believe the issue now is that there are many |
@Sam-Kruglov Thanks for the issue and the test/input! 🎉 |
@Sam-Kruglov Can you share your configuration, i.e. set of property names and replacement? My local test with that input works. Which version are you running? |
It's rc8. What exactly do you need apart what's on the screenshot? |
The predicate or set of property names. Can you try to write a test that reproduces this locally? Seems to work for me... 🤔 |
So the main function doesn't throw an error for you when you run it? For me it fails |
I think you are mixing up #681 and #686. #681 was for long property values. Thanks again for fixing this in #686 is for long property values containg (a lot of) escaped double qoutes. The unit test in the issue will allow you to reproduce the problem. |
Good point. Seems I was overwhelmed with StackOverflowErrors recently... |
The sample input has no double quotes. So I'm kind of lost why this one fails, especially since I can't reproduce it. |
@Sam-Kruglov I added your test input as an additional test case in #712 (aiming to fix #686). It already passed with on my machine before the changes though. Maybe you could verify if that branch builds locally for you. I'm out of ideas how to reproduce that, to be honest. |
I'm using jdk11, are you? I will try tomorrow |
Usually not, but I just tried and current master works as well with 11. |
@Sam-Kruglov Did you have a chance to try? |
I tried using open jdk 11.0.1 and 11.0.3 that I have installed. I launched it from intellij and got the stack overflow. Not sure how it works for you... |
oracle jdk 1.8.0_192 on a clean project with no dependencies - same stack overflow. Try copy-pasting the main method from my first message |
I took your code snippet and turned it into a test case without changing
the values. It's linked above and already in the master.
…On Fri, 14 Feb 2020, 16:40 Sam Kruglov, ***@***.***> wrote:
oracle jdk 1.8.0_192 on a clean project with no dependencies - same stack
overflow. Try copy-pasting the main method from my first message
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#689?email_source=notifications&email_token=AADI7HO24XJFTJEJKU3WMZTRC23NPA5CNFSM4KHHV3LKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELZNYOI#issuecomment-586341433>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADI7HLOUZMNWWCAPWLYHFDRC23NPANCNFSM4KHHV3LA>
.
|
So I took another look and your code sample inlined patterns that are no longer in use. There is a test case in the master that covers your sample input and it passes ✔️ logbook/logbook-json/src/test/java/org/zalando/logbook/json/JsonBodyFiltersTest.java Lines 173 to 182 in 54054d6
logbook/logbook-json/src/test/resources/huge-json-value.json Lines 1 to 4 in 54054d6
|
Thanks for taking a deep look |
Using a debugger, I built a function that will reproduce it (this is inside
com.monese.card.service.config.logbook.JsonBodyFilters#replace
insidedelegate
lambda):The text was updated successfully, but these errors were encountered: