gh #98 Extend kvp open function to also support URLs #99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
KarthikeyanR470
changed the title
kanjoe24
reviewed
Oct 9, 2025
src/ut_kvp.c
Outdated
| ut_kvp_instance_internal_t *pInternal = validateInstance(pInstance); | ||
| bool url = is_url(fileNameOrUrl); | ||
|
|
||
| if ((access(fileNameOrUrl, F_OK) != 0) && url == 0) |
Contributor
There was a problem hiding this comment.
can you reverse the order here.
url == 0 && (access(fileNameOrUrl, F_OK) != 0)
Contributor
There was a problem hiding this comment.
please add comments what is the purpose of the access(), your flow should be clear by reading the comments on what your doing.
kanjoe24
reviewed
Oct 9, 2025
tests/src/ut_test_kvp.c
Outdated
| #define KVP_VALID_TEST_SEQUENCE_INCLUDE_YAML "assets/include/sequence-include.yaml" | ||
| #define KVP_VALID_TEST_RESOLVE_YAML_TAGS_YAML "assets/yaml_tags.yaml" | ||
| #define KVP_VALID_TEST_RESOLVE_YAML_TAGS_IN_SEQUENCE_YAML "assets/yaml_tags_in_sequence.yaml" | ||
| #define KVP_VALID_TEST_URL_FILE "https://raw.githubusercontent.com/rdkcentral/ut-control/main/tests/src/assets/include/2s.yaml" |
Contributor
There was a problem hiding this comment.
KVP_VALID_TEST_URL_FILE -> rename this variable to KVP_VALID_TEST_URL
otherwise it will be misleading .
Ulrond
requested changes
Oct 10, 2025
src/ut_kvp.c
Outdated
| ut_kvp_instance_internal_t *pInternal = validateInstance(pInstance); | ||
| bool url = is_url(fileNameOrUrl); | ||
|
|
||
| if ((access(fileNameOrUrl, F_OK) != 0) && url == 0) |
Contributor
There was a problem hiding this comment.
please add comments what is the purpose of the access(), your flow should be clear by reading the comments on what your doing.
kanjoe24
reviewed
Oct 17, 2025
Contributor
|
Can you also share before and after logs for the latest changes please |
kanjoe24
added this to the
ut-control : 2.0.0 : Managing URL Support and Memory Ownership milestone
Ulrond
requested changes
Oct 24, 2025
Contributor
Ulrond
left a comment
Ulrond
left a comment
There was a problem hiding this comment.
coding standards and some bugs I think.
src/ut_kvp.c
Outdated
| { | ||
| return false; | ||
| } | ||
| return (strncmp(input, "http://", 7) == 0 || strncmp(input, "https://", 8) == 0); |
Contributor
There was a problem hiding this comment.
coding standard both the usage of MAGIC NUMBERS & the consolidation of functionality for add obfuscation.
Let the compiler consolidate the functionality with optimisation your requirement when you type in code is to make it debug able with breakpoints and also make it completely clear, even if it's longer to write.
all errors should also be checked in all functions with assert, since you've included it..
below is an example where you can code to set breakpoints on any of the return codes for debugging. All code should be walked through in the debugger to ensure it works even if you don't test the function, and you can't test your || case without separating it.
The compiler will optimise it, it's your not your as an engineer to do that, it's your job to make it very readable and clear.
#define UT_KVP_HTTPS_PREFIX "https:///"
#define UT_KVP_HTTP_PREFIX "http://"
#define UT_KVP_FILE_PREFIX "file://"
// Automatically calculate lengths by subtracting the '\0' null terminator
#define UT_KVP_HTTPS_PREFIX_LEN (sizeof(HTTPS_PREFIX) - 1)
#define UT_KVP_HTTP_PREFIX_LEN (sizeof(HTTP_PREFIX) - 1)
#define UT_KVP_FILE_PREFIX_LEN (sizeof(FILE_PREFIX) - 1)
....
/**
* @brief Checks if an input string is a known URL type.
* @param input The string to check.
* @return true if the string starts with http://, https://, or file://, false otherwise.
*/
static bool is_url(const char *input)
{
assert( input != NULL );
if (input == NULL)
{
return false;
}
// Check for http://
if (strncmp(input, UT_KVP_HTTP_PREFIX, UT_KVP_HTTP_PREFIX_LEN) == 0)
{
return true;
}
// Check for https://
if (strncmp(input, UT_KVP_HTTPS_PREFIX, UT_KVP_HTTPS_PREFIX_LEN) == 0)
{
return true;
}
// No matches found
return false;
}
src/ut_kvp.c
Outdated
| // -------------------- Handle URL-based input ----------------------- | ||
| if(bFilenameIsAUrl == true) | ||
| { | ||
| char yamlLocal[UT_KVP_MAX_ELEMENT_SIZE]; |
Contributor
There was a problem hiding this comment.
move the allocation tot he top of the function for visibly in what this function uses from local memory.
src/ut_kvp.c
Outdated
|
|
||
| // Dynamically allocate, since fy_document_build_from_malloc_string() | ||
| // will take ownership and free it internally. | ||
| char *yaml = strdup(yamlLocal); |
Contributor
There was a problem hiding this comment.
why do you strdup the input strings? when you've already got a local copy of it? using yamlLocal
src/ut_kvp.c
Outdated
| // will take ownership and free it internally. | ||
| char *yaml = strdup(yamlLocal); | ||
|
|
||
| if (!yaml) |
Contributor
There was a problem hiding this comment.
coding standards if (!) should be avoided it's not clear the intent, but based on the above comment I don't see why your duplicating something that's already duplicated?
if ( yaml == NULL )
{
...
}
``
src/ut_kvp.c
Outdated
| { | ||
| UT_LOG_ERROR("[%s] cannot be accesed", fileName); | ||
| // Skip "file://" | ||
| fileNameOrUrl = fileNameOrUrl + 7; |
Contributor
There was a problem hiding this comment.
fileNameOrUrl = fileNameOrUrl + UT_KVP_FILE_PREFIX_LEN;
src/ut_kvp.c
Outdated
| if (fileNameOrUrl == NULL) | ||
| { | ||
| UT_LOG_ERROR( "Invalid Param [fileName]" ); | ||
| UT_LOG_ERROR("Invalid Param [fileNameOrUrl]"); |
Contributor
There was a problem hiding this comment.
You should have a NULL error code UT_KVP_STATUS_NULL_PARAM ensure that INVALID_PARAM is used for bounds checking, and NULL param is used for NULLS
UT_LOG_ERROR("NULL PARAM [fileNameOrUrl]");
src/ut_kvp.c
Outdated
| } | ||
|
|
||
| if (access(fileName, F_OK) != 0) | ||
| ut_kvp_instance_internal_t *pInternal = validateInstance(pInstance); |
Contributor
There was a problem hiding this comment.
no error checks on pInstance which is also INVALID_INSTANCE & NULL PARAM possible.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the ut_kvp_open function to support URL-based input (HTTP/HTTPS) and file URI schemes, in addition to regular file paths. It also updates the error handling to use a more specific UT_KVP_STATUS_NULL_PARAM status code for null parameter cases.
- Adds support for HTTP, HTTPS, and file:// URI schemes in
ut_kvp_open - Changes null parameter error code from
UT_KVP_STATUS_INVALID_PARAMtoUT_KVP_STATUS_NULL_PARAM - Adds an HTTP server in the test runner script to enable local URL testing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/ut_kvp.c | Implements URL detection, file URI handling, and refactors ut_kvp_open to support URLs via ut_kvp_openMemory |
| include/ut_kvp.h | Updates function signature to use const char* and renames parameter to fileNameOrUrl |
| tests/src/ut_test_kvp.c | Adds test cases for HTTPS, HTTP, and file:// URI schemes and updates expected error code |
| tests/src/ut_control_test.sh | Adds HTTP server setup and cleanup logic to support URL testing |
Comments suppressed due to low confidence (1)
include/ut_kvp.h:79
- Documentation is outdated: the function now returns
UT_KVP_STATUS_NULL_PARAMfor null parameters (as shown in src/ut_kvp.c line 146), notUT_KVP_STATUS_INVALID_PARAM. The @RetVal documentation should be updated to reflect this change and add the missingUT_KVP_STATUS_NULL_PARAMretval.
* @retval UT_KVP_STATUS_INVALID_PARAM - One or more parameters are invalid (e.g., null pointer).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
Result |
kanjoe24
reviewed
Nov 5, 2025
Ulrond
requested changes
Nov 28, 2025
Contributor
Ulrond
left a comment
Ulrond
left a comment
There was a problem hiding this comment.
more tests required.
Something wrong the fact that your using access() on URL means either the test is not running or something is broken.
it should be failing..
|
|
||
| if(fy_document_resolve(srcDoc) != 0) | ||
| // Verify that the file is accessible | ||
| if (access(fileNameOrUrl, F_OK) != 0) |
Contributor
There was a problem hiding this comment.
doesn't this fail if the filename is a URL? Since the URL hasn't been downloaded you can't call access on it?
Why don't you get the error UT_KVP_STATUS_FILE_OPEN_ERROR on URL? in your testing is there a gap in your testing?
Contributor
Author
There was a problem hiding this comment.
The URL path does not reach the access() check.
The logic flow ensures that:
Line 151 in 60483b5
1.If the input is a URL (is_url(...) == true), the function immediately enters the 'if' block, constructs an in-memory YAML string, calls ut_kvp_openMemory(), and returns.
So URLs never fall through to the access() path.
2.The access(fileNameOrUrl, F_OK) check is executed only for non-URL file-based inputs.
So the code does not attempt to call access() on a URL, and therefore we do not get UT_KVP_STATUS_FILE_OPEN_ERROR for URL cases.
|
|
||
| UT_LOG_STEP("ut_kvp_open( pInstance, KVP_VALID_TEST_URI_FILE ) - Positive"); | ||
| status = ut_kvp_open( pInstance, KVP_VALID_TEST_URI_FILE); | ||
| UT_ASSERT( status == UT_KVP_STATUS_SUCCESS ); |
Contributor
There was a problem hiding this comment.
You need to add comments on the purpose of each test and what your trying to achieve.
Both Positive & Negative testing are always required.
And add comments to be the goals of the tests specifically.
- Valid Filenames, for open file:// &
- Valid Filanemes for https:// & http://
Malformed URLS, and invalid filenames must also be checked..
CAPS HTTPS/HTTP/FILE should also be failing and tested.
Ensure that both params are validated.
Whilst you can test the ut_kvp_open() function, how do you know it's read the right data in?
You need to add another tests to ut_kvp_open() & read data to prove it's working then ut_kvp_close().
That's a L2 test, and also if not already existing will require a test, I would suggest having a test for points 1) & 2) above.
Contributor
|
please resolve & review conversations and resolve as required.. |
Ulrond
reviewed
Nov 28, 2025
src/ut_kvp.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| if (strncmp(filename, "http:", 5) == 0 || strncmp(filename, "https:", 6) == 0) |
Contributor
There was a problem hiding this comment.
can you correct here also -> if (strncmp(filename, "http:", 5) == 0 || strncmp(filename, "https:", 6) == 0)
Magic numbers where you now have macros to cover it.
Contributor
Author
|
Before After |
kanjoe24
reviewed
Dec 11, 2025
| /**! | ||
| * @brief Opens and parses a Key-Value Pair (KVP) file into a KVP instance. | ||
| * | ||
| * This function opens the specified KVP file, reads its contents, and parses the key-value pairs into the given KVP instance. |
Contributor
There was a problem hiding this comment.
Also update about url here
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend the suport of the open function to support URL's.
https://github.com/rdkcentral/ut-control/blob/develop/include/ut_kvp.h
Transform function from
ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, char *fileName);
to
ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, char *fileNameOrUrl);
if the filename is prefixed with https/http:// then it's a url
if it's not prefixed then assume it's a file
if it's prefixed with file:// then assume it's a file.