Skip to content

gh #98 Extend kvp open function to also support URLs #99

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.

Sign up for GitHub

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

Open
KarthikeyanR470 wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

gh #98 Extend kvp open function to also support URLs #99

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
129893f
gh #98 updated ut_kvp_open
KarthikeyanR470 Oct 8, 2025
2731cc3
gh #98 updated the test
KarthikeyanR470 Oct 8, 2025
82d755c
gh #98 addressed review comments
KarthikeyanR470 Oct 10, 2025
4d0dde5
gh #98 addressed review comments
KarthikeyanR470 Oct 16, 2025
f362689
Merge branch 'develop' into feature/issue98_support_url
KarthikeyanR470 Oct 23, 2025
0540b76
gh #98 added free for string buffer
KarthikeyanR470 Oct 24, 2025
679e6f3
gh #98 updated param name of ut_kvp_open() in description
KarthikeyanR470 Oct 24, 2025
ff2c367
Merge branch 'develop' into feature/issue98_support_url
KarthikeyanR470 Oct 29, 2025
854e763
gh #98 addressed review comments
KarthikeyanR470 Oct 30, 2025
37babe1
gh #98 updated ut_kvp.c
KarthikeyanR470 Oct 30, 2025
889d5d5
gh #98 addressed copilot comments
KarthikeyanR470 Oct 31, 2025
5ea8838
Merge branch 'develop' into feature/issue98_support_url
KarthikeyanR470 Nov 7, 2025
9a66294
gh #98 updated the header file
KarthikeyanR470 Nov 7, 2025
155307c
Merge remote-tracking branch 'origin/develop' into feature/issue98_su…
KarthikeyanR470 Dec 9, 2025
60483b5
gh #98 Updated httpserver up logic in ut_control_test.sh
KarthikeyanR470 Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/ut_kvp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ void ut_kvp_destroyInstance(ut_kvp_instance_t *pInstance);
* @retval UT_KVP_STATUS_PARSING_ERROR - An error occurred while parsing the file contents.
* @retval UT_KVP_STATUS_INVALID_INSTANCE - The provided `pInstance` is not a valid KVP instance.
*/
ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, char *fileName);
ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, const char *fileNameOrUrl);

/**!
* @brief Opens and parses a memory block read from a Key-Value Pair (KVP) file into a KVP instance.
Expand Down
64 changes: 56 additions & 8 deletions src/ut_kvp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,45 +95,92 @@ void ut_kvp_destroyInstance(ut_kvp_instance_t *pInstance)
pInternal = NULL;
}

ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, char *fileName)
static bool is_url(const char *input)
{
if (input == NULL)
{
return false;
}
return (strncmp(input, "http://", 7) == 0 || strncmp(input, "https://", 8) == 0);
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

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;
}

}

ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, const char *fileNameOrUrl)
{
struct fy_node *node;
ut_kvp_instance_internal_t *pInternal = validateInstance(pInstance);

// Validate KVP instance handle
if (pInstance == NULL)
{
return UT_KVP_STATUS_INVALID_INSTANCE;
}

if (fileName == NULL)
// Validate fileNameOrUrl parameter
if (fileNameOrUrl == NULL)
{
UT_LOG_ERROR( "Invalid Param [fileName]" );
UT_LOG_ERROR("Invalid Param [fileNameOrUrl]");
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

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]");

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Oct 30, 2025

Choose a reason for hiding this comment

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

updated

return UT_KVP_STATUS_INVALID_PARAM;
}

if (access(fileName, F_OK) != 0)
ut_kvp_instance_internal_t *pInternal = validateInstance(pInstance);
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

no error checks on pInstance which is also INVALID_INSTANCE & NULL PARAM possible.


// Determine if input is a URL(e.g., http:// or https://)
bool bFilenameIsAUrl = is_url(fileNameOrUrl);

// -------------------- Handle URL-based input -----------------------
if(bFilenameIsAUrl == true)
{
char yamlLocal[UT_KVP_MAX_ELEMENT_SIZE];
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

move the allocation tot he top of the function for visibly in what this function uses from local memory.

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Oct 30, 2025

Choose a reason for hiding this comment

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

updated

snprintf(yamlLocal, sizeof(yamlLocal), "include: %s\n", fileNameOrUrl);

// Dynamically allocate, since fy_document_build_from_malloc_string()
// will take ownership and free it internally.
char *yaml = strdup(yamlLocal);
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

why do you strdup the input strings? when you've already got a local copy of it? using yamlLocal

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Oct 30, 2025

Choose a reason for hiding this comment

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

updated


if (!yaml)
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

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 )
{
...
}
``

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Oct 30, 2025

Choose a reason for hiding this comment

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

corrected

{
UT_LOG_ERROR("Memory allocation failed for yaml");
return UT_KVP_STATUS_PARSING_ERROR;
}

// Pass the dynamically allocated YAML string to openMemory() for parsing
return ut_kvp_openMemory(pInstance, yaml, strlen(yaml));
}

// ---------------------- Handle file-based input ----------------------
if (strncmp(fileNameOrUrl, "file://", 7) == 0)
{
// Skip "file://"
fileNameOrUrl = fileNameOrUrl + 7;
Copy link
Contributor

@Ulrond Ulrond Oct 24, 2025

Choose a reason for hiding this comment

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

fileNameOrUrl = fileNameOrUrl + UT_KVP_FILE_PREFIX_LEN;

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Oct 30, 2025

Choose a reason for hiding this comment

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

updated

}

// Verify that the file is accessible
if (access(fileNameOrUrl, F_OK) != 0)
Copy link
Contributor

@Ulrond Ulrond Nov 28, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Dec 10, 2025

Choose a reason for hiding this comment

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

The URL path does not reach the access() check.
The logic flow ensures that:

ut-control/src/ut_kvp.c

Line 151 in 60483b5

if(bFilenameIsAUrl == true)

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_ERROR("[%s] cannot be accesed", fileName);
UT_LOG_ERROR("[%s] cannot be accessed", fileNameOrUrl);
return UT_KVP_STATUS_FILE_OPEN_ERROR;
}

// If there’s already a handle, merge the new document into existing YAML
if(pInternal->fy_handle)
{
merge_nodes(fy_document_root(pInternal->fy_handle), fy_document_root(fy_document_build_from_file(NULL, fileName)));
merge_nodes(fy_document_root(pInternal->fy_handle), fy_document_root(fy_document_build_from_file(NULL, fileNameOrUrl)));
}
else
{
// Otherwise, create a new YAML document handle
pInternal->fy_handle = fy_document_create(NULL);
}

// Ensure the handle is valid
if (NULL == pInternal->fy_handle)
{
UT_LOG_ERROR("Unable to parse file/memory");
ut_kvp_close( pInstance );
return UT_KVP_STATUS_PARSING_ERROR;
}

struct fy_document *srcDoc = fy_document_build_from_file(NULL, fileName);
// Build a source YAML document from the file
struct fy_document *srcDoc = fy_document_build_from_file(NULL, fileNameOrUrl);

if(fy_document_resolve(srcDoc) != 0)
{
Expand All @@ -142,6 +189,7 @@ ut_kvp_status_t ut_kvp_open(ut_kvp_instance_t *pInstance, char *fileName)
return UT_KVP_STATUS_PARSING_ERROR;
}

// Process and copy nodes from source document into internal handle
node = process_node_copy(fy_document_root(srcDoc), pInternal->fy_handle, 0);

if (node == NULL)
Expand Down
18 changes: 17 additions & 1 deletion tests/src/ut_control_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,20 @@ cd "$(dirname "$0")"

export LD_LIBRARY_PATH=/usr/lib:/lib:/home/root:${MY_DIR}

./ut_control_test $@
# Start HTTP server in background
python3 -m http.server 8000 &
HTTP_PID=$!

# Register cleanup trap to stop HTTP server on exit
trap '
kill $HTTP_PID 2>/dev/null || true
wait $HTTP_PID 2>/dev/null || true
' EXIT

# Run test
./ut_control_test "$@"
UT_STATUS=$?

# Exit with same status as UT test
exit $UT_STATUS

18 changes: 18 additions & 0 deletions tests/src/ut_test_kvp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
#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_HTTPS "https://raw.githubusercontent.com/rdkcentral/ut-control/main/tests/src/assets/include/2s.yaml"
#define KVP_VALID_TEST_URI_HTTP "http://localhost:8000/assets/yaml_tags.yaml"
#define KVP_VALID_TEST_URI_FILE "file://assets/yaml_tags.yaml"

static ut_kvp_instance_t *gpMainTestInstance = NULL;
static UT_test_suite_t *gpKVPSuite = NULL;
Expand Down Expand Up @@ -125,6 +128,21 @@ void test_ut_kvp_open( void )
status = ut_kvp_open( pInstance, KVP_VALID_TEST_YAML_FILE);
UT_ASSERT( status == UT_KVP_STATUS_SUCCESS );

UT_LOG_STEP("ut_kvp_open( pInstance, KVP_VALID_TEST_URL_HTTPS ) - Positive");
status = ut_kvp_open( pInstance, KVP_VALID_TEST_URL_HTTPS);
printf("\nKVP Status = %d", status);
UT_ASSERT( status == UT_KVP_STATUS_SUCCESS );

UT_LOG_STEP("ut_kvp_open( pInstance, KVP_VALID_TEST_URI_HTTP ) - Positive");
status = ut_kvp_open( pInstance, KVP_VALID_TEST_URI_HTTP);
printf("\nKVP Status = %d", status);
UT_ASSERT( status == UT_KVP_STATUS_SUCCESS );

UT_LOG_STEP("ut_kvp_open( pInstance, KVP_VALID_TEST_URI_FILE ) - Positive");
status = ut_kvp_open( pInstance, KVP_VALID_TEST_URI_FILE);
printf("\nKVP Status = %d", status);
UT_ASSERT( status == UT_KVP_STATUS_SUCCESS );
Copy link
Contributor

@Ulrond Ulrond Nov 28, 2025

Choose a reason for hiding this comment

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

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.

  1. Valid Filenames, for open file:// &
  2. 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.

Copy link
Contributor Author

@KarthikeyanR470 KarthikeyanR470 Dec 9, 2025

Choose a reason for hiding this comment

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

Updated


UT_LOG_STEP("ut_kvp_open( pInstance, %s ) - Postive", KVP_VALID_TEST_NOT_VALID_YAML_FORMATTED_FILE);
status = ut_kvp_open( pInstance, KVP_VALID_TEST_NOT_VALID_YAML_FORMATTED_FILE);
UT_ASSERT( status == UT_KVP_STATUS_SUCCESS );
Expand Down