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

[BUG]Csv report generation had missing nested Fields #502

Merged
merged 11 commits into from
Jan 28, 2025

Conversation

sumukhswamy
Copy link
Contributor

@sumukhswamy sumukhswamy commented Jan 18, 2025

Description

On generating a CSV report from a saved search in Dashboards, nestsed fields for the index pattern was not getting parsed in the picture below
image

RCA:
the code piece handling the nested fields only checked if the fields had a key with a . in them, it was ignoring the fields which had an array of objects to be added to the CSV file, the fix handles the nested array objects which are needed as a part of the nested fields
The fix handles all nested fields which were getting truncated

image

Issues Resolved

#375

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.

}
flattenedValues[newKey].push(item[subKey]);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

seems this only does it one level, does it work if the data is more nested like

{
  "a": {
    "b": {
      "c": [
        {
          "d": [{ "e": 1 }, { "e": 2 }]
        },
        {
          "d": [{ "e": 3 }, { "e": 4 }]
        }
      ]
    }
  }
}

and could you add some unit tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have updated it

Copy link
Member

Choose a reason for hiding this comment

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

i don't really see any logic change in the new commits?

Copy link
Member

Choose a reason for hiding this comment

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

Adding more comments here:

  1. This may be still only working with one level of array nesting
  2. Best way to check is by adding multi-level nested array of objects
  3. Nit - Object.keys(data) is looped twice can just keep it one.

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: sumukhswamy <[email protected]>
server/routes/utils/test Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.22%. Comparing base (2f7ae07) to head (92af2c8).
Report is 28 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   71.05%   71.22%   +0.16%     
==========================================
  Files          31       31              
  Lines        2011     2054      +43     
  Branches      432      453      +21     
==========================================
+ Hits         1429     1463      +34     
- Misses        579      585       +6     
- Partials        3        6       +3     
Flag Coverage Δ
dashboards-report 71.22% <ø> (+0.16%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joshuali925 joshuali925 left a comment

Choose a reason for hiding this comment

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

as I commented here #502 (comment), this doesn't recurse, so it only flattens one level. For example, discover shows

{ "a": { "b": { "c": [ { "d": [{ "e": 1 }, { "e": 2 }] }, { "d": [{ "e": 3 }, { "e": 4 }] } ] } } }

as

a.b.c.d.e:1, 2, 3, 4

but reporting csv shows

_source.a.b.c
"[{""d"":[{""e"":1},{""e"":2}]},{""d"":[{""e"":3},{""e"":4}]}]"

but it's definitely better than not showing up at all

@joshuali925 joshuali925 merged commit c2ea8be into opensearch-project:main Jan 28, 2025
9 of 13 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 28, 2025
Signed-off-by: sumukhswamy <[email protected]>
(cherry picked from commit c2ea8be)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ps48 pushed a commit that referenced this pull request Jan 28, 2025
(cherry picked from commit c2ea8be)

Signed-off-by: sumukhswamy <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

3 participants