Add support for JSON-C library #10178
Conversation
This is in preparation for the subsequent commit adding test case for the prefs functionality to avoid building the code twice.
The CJSON_FOUND macro can be set despite the fact that should be used by the prefs functionality. Let's use the WITH_CJSON macro instead to avoid this situation. This is in preparation for the subsequent commit adding JSON-C support for the prefs functionality.
Let's add test case for SDL prefs functionality. This is in preparation for the subsequent commit adding JSON-C support to ensure it doesn't break something.
Let's use the cJSON_GetObjectItemCaseSensitive function instead of the cJSON_GetObjectItem function. This is in preparation for the subsequent commit adding JSON-C support to achieve consistent behavior. JSON-C doesn't support case-insensitive comparison. Although this is backwards incompatible change, it is not documented anywhere that it should be case-insensitive.
The SDL prefs functionality currently relies on the cJSON library. This is not available in RHEL for example. Let's add an option to build with the JSON-C library instead. Related: FreeRDP#10009
Let's use the cJSON_GetObjectItemCaseSensitive function instead of the cJSON_GetObjectItem function. This is in preparation for the subsequent commit adding JSON-C support to achieve consistent behavior. JSON-C doesn't support case-insensitive comparison. The protocol is case sensitive anyway.
Let's use the cJSON_GetStringValue function instead of dereference. This is in preparation for the subsequent commit adding JSON-C support to simplify things.
Currently, the `arm_handle_bad_request` function returns `FALSE` when the `cJSON_ParseWithLength` function fails to parse the message, but only when the `cJSON_GetErrorPtr` returns a valid pointer. It would be better to return regardless of the `cJSON_GetErrorPtr` return value.
The AAD functionality currently relies on the cJSON library. This is not available in RHEL for example. Let's add an option to build with the JSON-C library instead. Related: FreeRDP#10009
|
The implementation is pretty straightforward. It includes a test case for SDL prefs, however, the AAD authentication hasn't been tested. @fifthdegree , it seems that you are the author of the AAD support. Is there any chance, that you can test this, please? |
|
Refer to this link for build results (access rights to CI server needed): |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| static std::string item_to_str(cJSON *item, const std::string& fallback = "") | ||
| { | ||
| if (!item || !cJSON_IsString(item)) |
There was a problem hiding this comment.
warning: implicit conversion 'cJSON *' -> bool [readability-implicit-bool-conversion]
| if (!item || !cJSON_IsString(item)) | |
| if ((item == nullptr) || !cJSON_IsString(item)) |
| if (!item || !cJSON_IsString(item)) | ||
| return fallback; | ||
| auto str = cJSON_GetStringValue(item); | ||
| if (!str) |
There was a problem hiding this comment.
warning: implicit conversion 'char *' -> bool [readability-implicit-bool-conversion]
| if (!str) | |
| if (str == nullptr) |
| { | ||
| #if defined(WITH_CJSON) || defined(WITH_JSONC) | ||
| auto item = get_item(key); | ||
| if (!item || !cJSON_IsBool(item)) |
There was a problem hiding this comment.
warning: implicit conversion 'cJSON *' -> bool [readability-implicit-bool-conversion]
| if (!item || !cJSON_IsBool(item)) | |
| if ((item == nullptr) || !cJSON_IsBool(item)) |
| auto item = get_item(key); | ||
| if (!item || !cJSON_IsBool(item)) | ||
| return fallback; | ||
| return cJSON_IsTrue(item); |
There was a problem hiding this comment.
warning: implicit conversion 'cJSON_bool' (aka 'int') -> bool [readability-implicit-bool-conversion]
| return cJSON_IsTrue(item); | |
| return cJSON_IsTrue(item) != 0; |
| { | ||
| #if defined(WITH_CJSON) || defined(WITH_JSONC) | ||
| auto item = get_item(key); | ||
| if (!item || !cJSON_IsNumber(item)) |
There was a problem hiding this comment.
warning: implicit conversion 'cJSON *' -> bool [readability-implicit-bool-conversion]
| if (!item || !cJSON_IsNumber(item)) | |
| if ((item == nullptr) || !cJSON_IsNumber(item)) |
| #endif | ||
|
|
||
| auto bool_value_nonexistent = sdl_get_pref_bool("bool_key_nonexistent", false); | ||
| WINPR_ASSERT(!bool_value_nonexistent); |
There was a problem hiding this comment.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
WINPR_ASSERT(!bool_value_nonexistent);
^Additional context
winpr/include/winpr/assert.h:38: expanded from macro 'WINPR_ASSERT'
winpr_int_assert(#cond, __FILE__, __func__, __LINE__); \
^|
|
||
| auto array_value = sdl_get_pref_array("array_key", { "c", "b", "a" }); | ||
| #if defined(WITH_CJSON) || defined(WITH_JSONC) | ||
| WINPR_ASSERT(array_value.size() == 3 && array_value[0] == "a" && array_value[1] == "b" && |
There was a problem hiding this comment.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
WINPR_ASSERT(array_value.size() == 3 && array_value[0] == "a" && array_value[1] == "b" &&
^Additional context
winpr/include/winpr/assert.h:35: expanded from macro 'WINPR_ASSERT'
do \
^|
|
||
| auto array_value = sdl_get_pref_array("array_key", { "c", "b", "a" }); | ||
| #if defined(WITH_CJSON) || defined(WITH_JSONC) | ||
| WINPR_ASSERT(array_value.size() == 3 && array_value[0] == "a" && array_value[1] == "b" && |
There was a problem hiding this comment.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
WINPR_ASSERT(array_value.size() == 3 && array_value[0] == "a" && array_value[1] == "b" &&
^Additional context
winpr/include/winpr/assert.h:38: expanded from macro 'WINPR_ASSERT'
winpr_int_assert(#cond, __FILE__, __func__, __LINE__); \
^| #endif | ||
|
|
||
| auto array_value_nonexistent = sdl_get_pref_array("array_key_nonexistent", { "c", "b", "a" }); | ||
| WINPR_ASSERT(array_value_nonexistent.size() == 3 && array_value_nonexistent[0] == "c" && |
There was a problem hiding this comment.
warning: avoid do-while loops [cppcoreguidelines-avoid-do-while]
WINPR_ASSERT(array_value_nonexistent.size() == 3 && array_value_nonexistent[0] == "c" &&
^Additional context
winpr/include/winpr/assert.h:35: expanded from macro 'WINPR_ASSERT'
do \
^| #endif | ||
|
|
||
| auto array_value_nonexistent = sdl_get_pref_array("array_key_nonexistent", { "c", "b", "a" }); | ||
| WINPR_ASSERT(array_value_nonexistent.size() == 3 && array_value_nonexistent[0] == "c" && |
There was a problem hiding this comment.
warning: do not implicitly decay an array into a pointer; consider using gsl::array_view or an explicit cast instead [cppcoreguidelines-pro-bounds-array-to-pointer-decay]
WINPR_ASSERT(array_value_nonexistent.size() == 3 && array_value_nonexistent[0] == "c" &&
^Additional context
winpr/include/winpr/assert.h:38: expanded from macro 'WINPR_ASSERT'
winpr_int_assert(#cond, __FILE__, __func__, __LINE__); \
^
|
Sorry, I no longer have the proper setup (or spare time :) to test this. |
|
@ondrejholy thanks for the pull! |
| # JSONC_INCLUDE_DIRS - JSON-C include directories | ||
| # JSONC_LIBRARIES - libraries needed for linking | ||
|
|
||
| find_package(PkgConfig REQUIRED) |
There was a problem hiding this comment.
this will abort configuration on a system without pkg-config
There was a problem hiding this comment.
Some of the dependencies already require pkg-config, but it is true that they are not built by default, or only on Linux. So I will change that to not require it. Also the pkg_check_modules result was not used by mistake, so I will add the missing HINTS...
| # CJSON_INCLUDE_DIRS - cJSON include directories | ||
| # CJSON_LIBRARIES - libraries needed for linking | ||
|
|
||
| find_package(PkgConfig REQUIRED) |
There was a problem hiding this comment.
this will abort detection on a system without pkg-config
There was a problem hiding this comment.
I will change it here as well. I will also replace CJSON_LIBRARIES with cJSON_LIBRARIES as I have just found out that there is a bug on the master branch. The code currently relies on pkg-config results that use CJSON prefix and ignores results from find_library. The same applies for find_path.
|
|
||
| #elif defined(WITH_JSONC) | ||
|
|
||
| #include <json-c/json.h> |
There was a problem hiding this comment.
not sure this is an API we´d like to ship as our own like this in public headers.
might be better to use some thin wrapper and use our own naming scheme here to have that abstracted.
There was a problem hiding this comment.
other option would be to just move this to internals like before with cJSON so that we do not expose this ourselves. (you need to redo the used symbols for SDL client, but well...)
There was a problem hiding this comment.
Well, I expected that you wouldn't like this approach. I am just preparing a wrapper to avoid the code duplication...
| NAMES cjson | ||
| REQUIRED | ||
| ) | ||
| if (WITH_JSONC) |
There was a problem hiding this comment.
this should still search for the library (with REQUIRED keyword).
the detection before is used to determine the default value of the WITH_AAD option, but here we enforce the requirements.
There was a problem hiding this comment.
Isn't the message(FATAL_ERROR "AAD support requested but cJSON not found") enough?
The AAD authentication and SDL prefs currently relies on the cJSON library. This is not available in RHEL for example. Let's add an option to build with the JSON-C library instead.
Related: #10009