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

JsonView::GetBool returns wrong value #1731

Closed
2 tasks done
JanSiebert opened this issue Aug 10, 2021 · 6 comments · Fixed by #2852
Closed
2 tasks done

JsonView::GetBool returns wrong value #1731

JanSiebert opened this issue Aug 10, 2021 · 6 comments · Fixed by #2852
Assignees
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.

Comments

@JanSiebert
Copy link

Confirm by changing [ ] to [x] below to ensure that it's a bug:

Describe the bug
Aws::Utils::Json::JsonView::GetBool does not return the correct value if it has been set with Aws::Utils::Json::JsonValue::WithBool before. However, it works as expected if the value is serialized, deserialized and read afterwards.

SDK version number
1.8.182

Platform/OS/Hardware/Device
What are you running the sdk on?
Linux 13a2faedf94d 5.11.0-25-generic #27-Ubuntu SMP Fri Jul 9 23:06:29 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

To Reproduce (observed behavior)
The following test case fails:

#include <gtest/gtest.h>
#include <aws/core/utils/json/JsonSerializer.h>

TEST(JsonTest, BoolTest) {
  Aws::Utils::Json::JsonValue json;
  json.WithBool("yes", true);
  EXPECT_TRUE(json.View().GetBool("yes")); // ERROR

  Aws::Utils::Json::JsonValue json_parsed(json.View().WriteCompact());
  EXPECT_TRUE(json_parsed.View().GetBool("yes")); // OK
}

Expected behavior
I expected both test conditions to hold.

Additional context

bool JsonView::GetBool(const Aws::String& key) const
{
    assert(m_value);
    auto item = cJSON_GetObjectItemCaseSensitive(m_value, key.c_str());
    assert(item);
    return item->valueint != 0;
}

While valueint indeed is set during parsing [1], it is not used to represent the value (i.e. it is not set when creating a boolean value [2]). cJSON offers cJSON_IsBool, cJSON_IsFalse and cJSON_IsTrue. All these methods only look at the type field and not the valueint field.

[1] https://github.com/aws/aws-sdk-cpp/blob/main/aws-cpp-sdk-core/source/external/cjson/cJSON.cpp#L1348
[2] https://github.com/aws/aws-sdk-cpp/blob/main/aws-cpp-sdk-core/source/external/cjson/cJSON.cpp#L2423

@JanSiebert JanSiebert added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 10, 2021
@KaibaLopez
Copy link
Contributor

Hi @JanSiebert ,
Thanks for pointing this out, I'm able to reproduce this, we'll look into getting this fixed up.

@KaibaLopez KaibaLopez removed the needs-triage This issue or PR still needs to be triaged. label Aug 13, 2021
@jmklix jmklix added the p2 This is a standard priority issue label Mar 8, 2023
@jmklix jmklix self-assigned this Sep 13, 2023
@HumamHelfawi
Copy link

HumamHelfawi commented Nov 2, 2023

The problem is still there till now..

@triton3
Copy link

triton3 commented Jan 3, 2024

This issue is still present in v1.11

@jmklix jmklix mentioned this issue Feb 9, 2024
11 tasks
@jmklix
Copy link
Member

jmklix commented Feb 9, 2024

Here is the fix: #2852

@jmklix jmklix added the pending-release This issue will be fixed by an approved PR that hasn't been released yet. label Feb 9, 2024
@jmklix jmklix linked a pull request Feb 13, 2024 that will close this issue
11 tasks
@jmklix
Copy link
Member

jmklix commented Feb 13, 2024

This is fixed with the linked PR. Please let me know if you have any other problems.

@jmklix jmklix closed this as completed Feb 13, 2024
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p2 This is a standard priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants