<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 18, 2021 at 4:32 AM Quirin Pamp <<a href="mailto:pamp@atix.de">pamp@atix.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
Hi,</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
<br>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
it looks like there is now a commit on pulp_rpm that uses something named "urlpath_sanitize" instead of urljoin.</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
This is causing a major clash with this PR: <a href="https://github.com/pulp/pulp_rpm/pull/1896" id="gmail-m_8278429152740091121LPlnk919246" target="_blank">
https://github.com/pulp/pulp_rpm/pull/1896</a> which has replaced a lot of urljoin's with os.path.join.</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
<br>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
My problem is that since I am not the original author of this PR (I merely adopted it), I am not sure what the exact background of the switch to "os.path.join" is.</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
I presume, it was necessary (and perhaps trying to solve the same issue as the "urlpath_sanitize").</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
<br>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
I can of course rebase the path and test it out in either version (using urlpath_sanitize vs os.path.join), but if you have already put some recent thought into the intricacies of urljoin, I would appreciate your input before I do. 😉</div></div></blockquote><div><br></div><div>Well, this raises a really good point.</div><div><br></div><div>Both of these PRs address the fact that the code was using 'urljoin' when all it really wanted (in almost all cases) was to join a base-url (like, say, "remote_url") and a relative-path fragment. Places where base-url didn't end in '/' caused a variety of subtle problems.</div><div><br></div><div>os.path.join() does, in fact, address this problem - *on Linux systems*. If you're running python on a system that uses something other than '/' for the path-separator (like, say, Windows' '\') - then os.path.join() can do Bad Things to your urls.</div><div><br></div><div>I built url_sanitize() to get what was wanted from the current uses of urljoin(), while avoiding the pitfalls it was causing. Replacing it with os.path.join() fixes that set of problems as well, but it feels like just setting a different landmine to blow us up in the future, if/when we support running Pulp3 on something non-Linux.</div><div><br></div><div>And in addition, the entities we're dealing with here, generally *aren't filesystem paths*. It's misleading the future developer to imply that's what they are.</div><div><br></div><div>Anyway, those were my thoughts when I went the url_sanitize path. More and/or better ones, always welcome!</div><div><br></div><div>G</div><div><br></div><div>(My only other thoughts here are that the semantics of 'urljoin()' are WAY too subtle, and we are not alone in writing code that is eventually inadvertently broken by them. I honestly cannot recall ever seeing urljoin() used in a place where those semantics were actually desired. But that's just me being grumpy...)</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)"></div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
<br>
</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
Regards,</div>
<div style="font-family:Calibri,Arial,Helvetica,sans-serif;font-size:12pt;color:rgb(0,0,0)">
Quirin (quba42)<br>
</div>
<div id="gmail-m_8278429152740091121appendonsend"></div>
<hr style="display:inline-block;width:98%">
<div id="gmail-m_8278429152740091121divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> <a href="mailto:pulp-dev-bounces@redhat.com" target="_blank">pulp-dev-bounces@redhat.com</a> <<a href="mailto:pulp-dev-bounces@redhat.com" target="_blank">pulp-dev-bounces@redhat.com</a>> on behalf of Grant Gainey <<a href="mailto:ggainey@redhat.com" target="_blank">ggainey@redhat.com</a>><br>
<b>Sent:</b> 15 March 2021 20:43<br>
<b>To:</b> Pulp-dev <<a href="mailto:pulp-dev@redhat.com" target="_blank">pulp-dev@redhat.com</a>><br>
<b>Subject:</b> [Pulp-dev] pulp_rpm and The Pitfalls of urljoin()</font>
<div> </div>
</div>
<div>
<div dir="ltr">Hey folks,
<div><br>
</div>
<div>I was looking at <a href="https://pulp.plan.io/issues/7995" target="_blank">https://pulp.plan.io/issues/7995</a> with an eye to fixing it for 3.12. The underlying problem can be summed up as "urljoin doesn't do what you expect if 'base' doesn't end in a '/'"[0][1]. Using
urljoin() as if all it does is "concatenate these two strings with a '/' between them" is a pretty common misuse that works 'most of the time', alas.</div>
<div><br>
</div>
<div>Looking around to see if we do this anywhere else, I find that pulp_rpm uses urljoin() in a number of places that I think might be subject to the same unintended problem. However, the semantics of sync'ing RPM repositories is even
<b>more</b> nuanced than urljoin()'s is!</div>
<div><br>
</div>
<div>I am tempted to replace urljoin() with just straightforward path-creation. Here's the list of places in pulp_rpm non-test python that uses it currently:</div>
<div><br>
</div>
<div><font size="1" face="monospace">(pulp3) (master) ~/github/Pulp3/pulp_rpm $ find . -name \*.py | grep -v tests | xargs grep -n "urljoin("
<br>
./pulp_rpm/app/kickstart/treeinfo.py:23: url=urljoin(remote_url, namespace), silence_errors_for_response_status_codes={403, 404}<br>
./pulp_rpm/app/models/repository.py:355: gpgkey_path = urllib.parse.urljoin(<br>
./pulp_rpm/app/models/repository.py:358: gpgkey_path = urllib.parse.urljoin(gpgkey_path, self.base_path, True)<br>
./pulp_rpm/app/tasks/synchronizing.py:101: downloader = remote.get_downloader(url=urljoin(url, "repodata/repomd.xml"))<br>
./pulp_rpm/app/tasks/synchronizing.py:138: downloader = remote.get_downloader(url=urljoin(remote_url, "repodata/repomd.xml"))<br>
./pulp_rpm/app/tasks/synchronizing.py:243: new_url = urljoin(remote_url, path)<br>
./pulp_rpm/app/tasks/synchronizing.py:430: url=urljoin(self.data.remote_url, "repodata/repomd.xml")<br>
./pulp_rpm/app/tasks/synchronizing.py:460: url=urljoin(self.data.remote_url, self.treeinfo["filename"]),<br>
./pulp_rpm/app/tasks/synchronizing.py:470: url=urljoin(self.data.remote_url, path),<br>
./pulp_rpm/app/tasks/synchronizing.py:599: url = urljoin(self.data.remote_url, package.location_href)<br>
./pulp_rpm/app/tasks/synchronizing.py:722: repodata_url = urljoin(self.data.remote_url, record.location_href)<br>
./pulp_rpm/app/tasks/synchronizing.py:726: self.data.updateinfo_url = urljoin(self.data.remote_url, record.location_href)<br>
./pulp_rpm/app/tasks/synchronizing.py:731: comps_url = urljoin(self.data.remote_url, record.location_href)<br>
./pulp_rpm/app/tasks/synchronizing.py:735: self.data.modules_url = urljoin(self.data.remote_url, record.location_href)<br>
./pulp_rpm/app/tasks/synchronizing.py:743: url=urljoin(self.data.remote_url, record.location_href),<br>
./pulp_rpm/app/downloaders.py:84: url = urljoin(self.url, auth_param)<br>
(pulp3) (master) ~/github/Pulp3/pulp_rpm $</font></div>
<div> <br>
</div>
<div>
<div>Any thoughts before I dive down this rabbithole? I'm afraid I don't even have a pocketwatch...</div>
<div><br>
</div>
<div>G</div>
<div><br>
</div>
<div>[0] <a href="https://docs.python.org/2/library/urlparse.html#urlparse.urljoin" target="_blank">https://docs.python.org/2/library/urlparse.html#urlparse.urljoin</a></div>
<div>[1] <a href="https://stackoverflow.com/a/10893427" target="_blank">https://stackoverflow.com/a/10893427</a></div>
-- <br>
<div dir="ltr">
<div dir="ltr">
<div>
<div dir="ltr">
<div>Grant Gainey</div>
<div>Principal Software Engineer, Red Hat System Management Engineering</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</div>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Grant Gainey</div><div>Principal Software Engineer, Red Hat System Management Engineering</div></div></div></div></div></div>