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 CSV/RAW output header being application/json rather than plain/text #1779

Merged

Conversation

matthewryanwells
Copy link
Contributor

Description

When making SQL calls with csv or raw outputs the formatting would be incorrect as the header sets the format to application/json rather than plain/text like the legacy engine did.

This PR adds a format getter to both response formatters to allow it to be set correctly.

Issues Resolved

#1572

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Yury-Fridlyand and others added 3 commits June 22, 2023 10:39
* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
…t-fix

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #1779 (294dd8f) into main (f6e2a97) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #1779   +/-   ##
=========================================
  Coverage     97.29%   97.29%           
- Complexity     4408     4410    +2     
=========================================
  Files           388      388           
  Lines         10944    10946    +2     
  Branches        774      774           
=========================================
+ Hits          10648    10650    +2     
  Misses          289      289           
  Partials          7        7           
Flag Coverage Δ
sql-engine 97.29% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rotocol/response/format/FlatResponseFormatter.java 100.00% <100.00%> (ø)
...rotocol/response/format/JsonResponseFormatter.java 100.00% <100.00%> (ø)

Copy link
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@acarbonetto
Copy link
Collaborator

@matthewryanwells have we determined if this fixes nested and objects in the Dashboard too?

@matthewryanwells
Copy link
Contributor Author

@matthewryanwells have we determined if this fixes nested and objects in the Dashboard too?

@acarbonetto I can confirm that this fixes the nested, objects, and any other issues with output as the problem was how csv and raw were being handled, rather than the specific thing being processed.

@Yury-Fridlyand Yury-Fridlyand merged commit 1ec696d into opensearch-project:main Jun 27, 2023
16 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the integ-csv-raw-output-fix branch June 27, 2023 21:14
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 27, 2023
…xt (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit 1ec696d)
Yury-Fridlyand pushed a commit that referenced this pull request Jun 28, 2023
* Fix CSV/RAW output header being application/json rather than plain/text (#1779)

* Fix CI (#1760)

* Fix ML-commons missing dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Fix `mockito` dependency.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

* Revert changes in `:opensearch` since it is not needed anymore.

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* Fix CSV/RAW outputting wrong format (#279)

* Fixed bug where CSV/RAW outputs as JSON rather than plain text

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Matthew Wells <matthew.wells@improving.com>
Co-authored-by: Yury-Fridlyand <yury.fridlyand@improving.com>
(cherry picked from commit 1ec696d)

* updated tests

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* updated tests to return name

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* changed tests to return value

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

* removed unneeded imports

Signed-off-by: Matthew Wells <matthew.wells@improving.com>

---------

Signed-off-by: Matthew Wells <matthew.wells@improving.com>
@acarbonetto acarbonetto added the v2.9.0 v2.9.0 label Jul 4, 2023
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.

[BUG] nested/object query with raw or csv format inserts unexpected newlines
4 participants