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

Update tests to include jsoncpp trait #320

Merged
merged 6 commits into from
Dec 21, 2023

Conversation

prince-chrismc
Copy link
Collaborator

@cjserio I believe there is a small bug 🐛 with the behavior regarding integers (NumbericDatetime from RFC 7519) being mistaken for number (floats, etc). This is something most other libraries have with the translation from Javascript to C++ but I am not sure how it works for JSONCPP so I would appreciate any help looking into this 🙏

@cjserio
Copy link
Contributor

cjserio commented Dec 21, 2023

Hmm the only error i see in the test is:

/home/runner/work/jwt-cpp/jwt-cpp/tests/traits/OspJsoncppTest.cpp:15: Failure Expected equality of these values: integer.get_type() Which is: 4-byte object <02-00 00-00> jwt::json::type::integer Which is: 4-byte object <01-00 00-00>

Which looks like the test failing is:

const auto integer = jwt::basic_claim<jwt::traits::open_source_parsers_jsoncpp>(159816816); ASSERT_EQ(integer.get_type(), jwt::json::type::integer);

Is that what you're referring to?

@prince-chrismc
Copy link
Collaborator Author

yes that exactly it! Thanks for being so quick ❤️

jwt::json::type::integer == 1
jwt::json::type::number == 2

@cjserio
Copy link
Contributor

cjserio commented Dec 21, 2023

Facepalm for JsonCPP:

bool Value::isDouble() const {
  return type() == intValue || type() == uintValue || type() == realValue;
}

bool Value::isNumeric() const { return isDouble(); }

I think I just need to be more strict about the order I check these in...Where should I push a fix for this?

@cjserio
Copy link
Contributor

cjserio commented Dec 21, 2023

Actually, here's the patch if you want to just apply it. Otherwise let me know what to branch off of and where to put the fix:

git diff
diff --git a/include/jwt-cpp/traits/open-source-parsers-jsoncpp/traits.h b/include/jwt-cpp/traits/open-source-parsers-jsoncpp/traits.h
index 1d4c0c7..3598a7d 100644
--- a/include/jwt-cpp/traits/open-source-parsers-jsoncpp/traits.h
+++ b/include/jwt-cpp/traits/open-source-parsers-jsoncpp/traits.h
@@ -66,10 +66,10 @@ namespace jwt {
                                        return type::array;
                                else if (val.isString())
                                        return type::string;
+                else if (val.isInt())
+                    return type::integer;
                                else if (val.isNumeric())
                                        return type::number;
-                               else if (val.isInt())
-                                       return type::integer;
                                else if (val.isBool())
                                        return type::boolean;
                                else if (val.isObject())

@prince-chrismc
Copy link
Collaborator Author

@prince-chrismc prince-chrismc merged commit ed98dc1 into Thalhammer:master Dec 21, 2023
55 of 56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants