common: inf and NaN are invalid in JSON by sajibreadd · Pull Request #57695 · ceph/ceph

sajibreadd · 2024-05-24T11:29:18Z

Fixes: https://tracker.ceph.com/issues/66215

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

src/common/Formatter.cc

sebastian-philipp · 2024-05-24T11:42:08Z

@batrick do you want to review this?

src/test/common/test_json_formatter.cc

src/common/Formatter.cc

src/common/Formatter.h

src/common/Formatter.cc

batrick

Commit title JSON Parser does not allow inf and NaN. Converted it to null. to common/Formatter: dump inf/nan as strings

sajibreadd · 2024-05-29T11:02:39Z

Is this PR going for the test? Also I think we need to backport this PR. #57392
PR is a dependency for my commit to be backported. So we need to cherr-pick both commits.

src/test/common/test_json_formatter.cc

batrick · 2024-05-31T16:09:09Z

jenkins test make check arm64

patrakov · 2024-05-31T16:44:26Z

src/common/Formatter.cc

-template <class T>
+void JSONFormatter::add_value(std::string_view name, double val) {
+  CachedStackStringStream css;
+  if (std::isinf(val) || std::isnan(val)) {


Would the shorter !std::isfinite(val) form work here?

Sorry, missed your comment somehow.
It does work but do you have any special observation for using that instead of std::isinf(val) ?

It's shorter, expresses the intent more directly, enumerates goodness not badness,... i.e. purely aesthetical reasons.

sajibreadd · 2024-06-17T07:34:38Z

jenkins test make check

sajibreadd · 2024-06-17T10:03:21Z

jenkins test docs

sajibreadd · 2024-06-17T10:05:47Z

jenkins test api

sajibreadd · 2024-06-17T10:06:11Z

jenkins test windows

sajibreadd · 2024-06-17T10:59:06Z

jenkins test make check arm64

sajibreadd · 2024-06-17T13:45:18Z

jenkins test api

batrick

minor nits, otherwise lgtm

batrick · 2024-06-17T21:23:37Z

src/common/Formatter.h

+    void add_value(std::string_view name, double val);

-    template <class T>
+    template <typename T>


undo please

batrick · 2024-06-17T21:23:43Z

src/common/Formatter.cc

  }
 }

-template <class T>


undo this change

batrick · 2024-06-17T21:24:07Z

src/test/common/test_json_formatter.cc

+  EXPECT_EQ(parser.find_obj("negative_infinity")->get_data(), "null");
+  EXPECT_EQ(parser.find_obj("nan_val")->get_data(), "null");
+  EXPECT_EQ(parser.find_obj("nan_val_alt")->get_data(), "null");
+}


missing terminal newline

Fixes: https://tracker.ceph.com/issues/66215 Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

sajibreadd · 2024-06-21T10:36:06Z

jenkins test make check

sajibreadd · 2024-06-21T15:57:18Z

jenkins test make check

sajibreadd · 2024-06-21T15:59:19Z

jenkins test api

sajibreadd · 2024-06-25T08:01:16Z

@batrick Is there anything I need to do in this PR?

ifed01 · 2024-07-15T07:53:25Z

src/test/common/test_json_formatter.cc

+  EXPECT_EQ(parser.find_obj("negative_infinity")->get_data(), "null");
+  EXPECT_EQ(parser.find_obj("nan_val")->get_data(), "null");
+  EXPECT_EQ(parser.find_obj("nan_val_alt")->get_data(), "null");
+}


please verify double_max/double_min values as well

batrick · 2024-07-15T14:21:18Z

@sajibreadd did you mean to close this? Please reopen?

sajibreadd · 2024-07-15T14:25:45Z

@sajibreadd did you mean to close this? Please reopen?

Here is some mistake I did. I merged main into the branch by mistake(by syncing the fork by mistake). As there is a lot of commit to drop that's why here is the things I have done,

created a new branch.
cherrypicked the commit.
pushed the branch upstream
deleted the old branch named wip-66215
renamed the newly created branch wip-66215

Now github is not allowing me to reopen the PR. Apologies for the mistakes. Now what should be the best things to do? create another PR and have reference of this PR?

github-actions bot added the common label May 24, 2024

sebastian-philipp reviewed May 24, 2024

View reviewed changes

src/common/Formatter.cc Show resolved Hide resolved

sajibreadd force-pushed the wip-66215 branch from 40b7b31 to 9268c2d Compare May 24, 2024 13:18

github-actions bot added the tests label May 24, 2024

batrick requested changes May 24, 2024

View reviewed changes

src/test/common/test_json_formatter.cc Outdated Show resolved Hide resolved

src/common/Formatter.cc Show resolved Hide resolved

src/common/Formatter.h Show resolved Hide resolved

src/common/Formatter.cc Show resolved Hide resolved

sajibreadd force-pushed the wip-66215 branch 2 times, most recently from 3468f5a to 81e2164 Compare May 24, 2024 17:35

batrick requested changes May 24, 2024

View reviewed changes

sajibreadd force-pushed the wip-66215 branch 2 times, most recently from d3a9b4e to b3beee4 Compare May 27, 2024 15:01

sajibreadd mentioned this pull request May 28, 2024

common/Formatter: use CachedStackStringStream for efficiency #57392

Merged

14 tasks

This was referenced May 29, 2024

squid: inf and NaN are invalid in JSON #57778

Closed

squid: inf and NaN are invalid in JSON #57779

Closed

quincy: common/Formatter: dump inf/nan as null #57780

Closed

reef: common/Formatter: dump inf/nan as null #57781

Closed

batrick requested changes May 30, 2024

View reviewed changes

src/test/common/test_json_formatter.cc Show resolved Hide resolved

sajibreadd force-pushed the wip-66215 branch from b3beee4 to 76954f7 Compare May 31, 2024 10:09

batrick approved these changes May 31, 2024

View reviewed changes

batrick requested review from patrakov and sebastian-philipp May 31, 2024 16:07

batrick added the needs-qa label May 31, 2024

batrick changed the title ~~inf and NaN are invalid in JSON~~ common: inf and NaN are invalid in JSON May 31, 2024

batrick requested review from a team and removed request for patrakov and sebastian-philipp May 31, 2024 16:07

patrakov reviewed May 31, 2024

View reviewed changes

sajibreadd force-pushed the wip-66215 branch from 76954f7 to aeccca7 Compare June 7, 2024 12:29

batrick requested changes Jun 17, 2024

View reviewed changes

common/Formatter: dump inf/nan as null

e618095

Fixes: https://tracker.ceph.com/issues/66215 Signed-off-by: Md Mahamudur Rahaman Sajib <mahamudur.sajib@croit.io>

sajibreadd force-pushed the wip-66215 branch from aeccca7 to e618095 Compare June 21, 2024 06:24

ifed01 reviewed Jul 15, 2024

View reviewed changes

Merge branch 'ceph:main' into wip-66215

7f23b06

sajibreadd closed this Jul 15, 2024

sajibreadd deleted the wip-66215 branch July 15, 2024 14:12

sajibreadd mentioned this pull request Jul 15, 2024

common/Formatter: dump inf/nan as null #58598

Merged

14 tasks

Conversation

sajibreadd commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

sebastian-philipp commented May 24, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

sajibreadd commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

batrick commented May 31, 2024

Uh oh!

patrakov May 31, 2024

Choose a reason for hiding this comment

Uh oh!

sajibreadd Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

patrakov Jun 7, 2024

Choose a reason for hiding this comment

Uh oh!

sajibreadd commented Jun 17, 2024

Uh oh!

sajibreadd commented Jun 17, 2024

Uh oh!

sajibreadd commented Jun 17, 2024

Uh oh!

sajibreadd commented Jun 17, 2024

Uh oh!

sajibreadd commented Jun 17, 2024

Uh oh!

sajibreadd commented Jun 17, 2024

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

batrick Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

batrick Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

batrick Jun 17, 2024

Choose a reason for hiding this comment

Uh oh!

sajibreadd commented Jun 21, 2024

Uh oh!

sajibreadd commented Jun 21, 2024

Uh oh!

sajibreadd commented Jun 21, 2024

Uh oh!

sajibreadd commented Jun 25, 2024

Uh oh!

ifed01 Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

batrick commented Jul 15, 2024

Uh oh!

sajibreadd commented Jul 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sajibreadd commented May 24, 2024 •

edited

Loading

sajibreadd commented May 29, 2024 •

edited

Loading