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

Allow Query String Parameters in the URL #469

Closed
wants to merge 1 commit into from

Conversation

adithyaj
Copy link

@adithyaj adithyaj commented Mar 5, 2024

As discussed in #467, when the baseURL has appended query string parameters, tdnf does not recognize this as a query string parameter and treats this as a standard baseURL and appends paths like the repomd.xml to the end of this. For query strings, the expectation is that these parameters would appear at the very end of the URL, after the paths have been appended.

The following is a change to the common/utils.c path to update the URL in the event we see a query string.

@vmwclabot
Copy link
Member

@adithyaj, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

@@ -836,6 +838,19 @@ TDNFJoinPath(char **ppszPath, ...)
* before stripping all leading slashes */
if (i == 0)
{
/* we also check to see if the base path is a remote URL; for URLs
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like that you are using TDNFJoinPath() for this, which is a generic function to join path elements (much like python's os.path.join(). I think it's not very efficient, because this code will now be used in all places where we join paths, which are mostly local.

Instead, I think we should create another version of this function that is only used where we need this functionality of preserving the URL query string.

Copy link
Contributor

@oliverkurth oliverkurth Mar 16, 2024

Choose a reason for hiding this comment

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

@adithyaj , are you going to address my comments?

@oliverkurth
Copy link
Contributor

Is this still an issue? @adithyaj , will you address my review comments? Otherwise, I will close this.

@oliverkurth
Copy link
Contributor

Closing this, looks like the issue is stalled.

Feel free to reopen, or create another PR with my comments addressed.

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.

3 participants