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

Ensure array_type has front() method + fix array_type on jsoncpp #361

Merged
merged 13 commits into from
Nov 17, 2024

Conversation

omar-mohamed-khallaf
Copy link
Contributor

Replace front() with [0] in get_x5c_key_value as front is not supported by jsoncpp on array types.

Replace the `front()` with `[0]` as front is not supported by jsoncpp on
array types.

Signed-off-by: Omar Mohamed <[email protected]>
@prince-chrismc
Copy link
Collaborator

Would it be possible to implement the missing function in the trait definition?

Extending,

class array_type : public Json::Value {

Also it seems we are missing this function as a requirement for the json array

struct is_valid_json_array {

@omar-mohamed-khallaf
Copy link
Contributor Author

Is front() the missing function for the second issue, or something else?

@prince-chrismc
Copy link
Collaborator

prince-chrismc commented Sep 23, 2024

Yes, front() Should be checked.

Might also be worth checking if the unit tests are missing a case 🤔 we just dint run the unit tests with JSON libraries

Notes:

auto x5c = jwk.get_x5c_key_value();

ASSERT_EQ(jwk3.get_x5c_key_value(), "1");

@omar-mohamed-khallaf
Copy link
Contributor Author

Yes, front() Should be checked.

I added this line !std::is_same<decltype(std::declval<array_type>().front()), void>::value to is_valid_json_array, is it ok?

std::is_member_function_pointer had problems resolving the overloaded front function, and I had no idea how to solve it.

@omar-mohamed-khallaf omar-mohamed-khallaf changed the title Use operator[] instead of front() in get_x5c_key_value to support jsoncpp Ensure array_type has front() method Sep 30, 2024
Copy link
Collaborator

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

Amazing work, thanks for helping to fix this!

@prince-chrismc
Copy link
Collaborator

Looks like a type error in the Danieljson changes from the CI

@omar-mohamed-khallaf
Copy link
Contributor Author

Something is wrong in the current implementation.

static object_type as_object(const json& val) {
	if (val.type() != jsoncons::json_type::object_value) throw std::bad_cast();
	return object_type(val.object_value());
}

static array_type as_array(const json& val) {
	if (val.type() != jsoncons::json_type::array_value) throw std::bad_cast();
	return val.array_value();
}

These two functions use val.object_value() and val.array_value(), but I can't find any reference to these functions in the jsoncons library.

@omar-mohamed-khallaf
Copy link
Contributor Author

The problem existed before the current modifications.
Should it be fixed in a new PR?

@prince-chrismc prince-chrismc changed the title Ensure array_type has front() method Ensure array_type has front() method + fix array_type on jsoncpp Nov 17, 2024
@prince-chrismc prince-chrismc merged commit 71c3d36 into Thalhammer:master Nov 17, 2024
60 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