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.

Sign up for GitHub

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

Jump to bottom

SOLR-17009 json.wrf parameter ignored in JacksonJsonWriter #1977

Merged
merged 3 commits into from
Oct 4, 2023
Merged

SOLR-17009 json.wrf parameter ignored in JacksonJsonWriter #1977

merged 3 commits into from
Oct 4, 2023

Conversation

stillalex
Copy link
Member

https://issues.apache.org/jira/browse/SOLR-17009

Description

Please provide a short description of the changes you're making with this pull request.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.
  • I have added documentation for the Reference Guide

@epugh
Copy link
Contributor

epugh commented Oct 3, 2023

Makes sense... I sorta wish we had a unit test. I noticed that the other Writers all have reasonable tests. But the Json ones don't.. JsonWriterTest is kind of mess compared to TestCSVResponseWriter and friends...

@stillalex
Copy link
Member Author

I agree! I was looking for an example of how to test this and there were not too many...

@stillalex
Copy link
Member Author

stillalex commented Oct 4, 2023 •
edited
Loading

I added a test but GH is having a moment and taking some time to show the latest.

for future reference this is the comparison from the 2 json writers

  • JacksonJsonWriter
testFun( {
  \"param0\":\"v0\",
  \"param1\":42
} )
  • JSONWriter
testFun({
  \"param0\":\"v0\",
  \"param1\":42})

epugh
epugh reviewed Oct 4, 2023
View reviewed changes
solr/core/src/test/org/apache/solr/response/JSONWriterTest.java
@@ -314,4 +315,35 @@ public void testConstantsUnchanged() {
assertEquals("arrntv", JSONWriter.JSON_NL_ARROFNTV);
assertEquals("json.wrf", JSONWriter.JSON_WRAPPER_FUNCTION);
}

@Test
public void testWfrJacksonJsonWriter() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love the test... Would it make sense to break out these tests? Move to the JacksonJsonResponseWriterTest.java and JsonResponseWriterTest.java?

@stillalex stillalex merged commit 540f61c into apache:main Oct 4, 2023
2 checks passed
@stillalex stillalex deleted the SOLR-17009-jsonwrf branch October 4, 2023 15:32
stillalex added a commit that referenced this pull request Oct 4, 2023
@stillalex
SOLR-17009 json.wrf parameter ignored in JacksonJsonWriter (#1977)
16f7520 
(cherry picked from commit 540f61c)
stillalex added a commit that referenced this pull request Oct 4, 2023
@stillalex
SOLR-17009 json.wrf parameter ignored in JacksonJsonWriter (#1977)
7998823 
(cherry picked from commit 540f61c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epugh epugh epugh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stillalex @epugh