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

WIP: Implement HTTP #468

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

WIP: Implement HTTP #468

wants to merge 5 commits into from

Conversation

pjbull
Copy link
Member

@pjbull pjbull commented Sep 1, 2024

Initial pass at http.

  • implemented with standard library
  • tested against Python http.server
  • will try writing/deleting files with PUT/DELETE in case your server allows it
  • will try parsing "directories" from the returned HTML; seems like this grabs the right things from python's http.server and also looks like it should work for apache and nginx file servers from my googling. (allows user to override this method for their own server)
  • Bonus fix, use .anchor instead of .cloud_prefix where appropriate. For cloud providers, .anchor is the same as .cloud_prefix (e.g., s3://). For http, the anchor should be the server http://example.com and the .cloud_prefix should be http://. Constructing new paths should use .anchor

Limitations/caveats:

  • Assumes that url must have suffix (e.g., http://example.com/file.txt) to be a file; if no suffix, assumes dir. This is definitely not true of real-world URLs, but maybe is an ok assumption for anything serving files?

Left to do:

  • Add https
  • Expose all the parsed url components publicly (not behind HttpPath._url).
  • Update default file + dir function to be based on a trailing slash
  • Allow also overriding the file vs. dir function
  • Add tests for http specific functions
  • Turn off noisy logs for test http server by default
  • Add tests that url search strings and fragments are persisted correctly
  • Documentation (table, caveats, docstrings, headers, auth, files v dirs, etc.)
  • HTTP Test suite often passes locally, but not always; need to debug and make less flaky

Closes #455

Copy link
Contributor

github-actions bot commented Sep 1, 2024

@github-actions github-actions bot temporarily deployed to pull request September 1, 2024 11:22 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 1, 2024 11:29 Inactive
@jayqi
Copy link
Member

jayqi commented Sep 4, 2024

Bonus fix, use .anchor instead of .cloud_prefix where appropriate. For cloud providers, .anchor is the same as .cloud_prefix (e.g., s3://). For http, the anchor should be the server http://example.com and the .cloud_prefix should be http://. Constructing new paths should use .anchor

We shouldn't call this "anchor". Anchor has a different meaning in a close enough context that I think this will be confusing. In particular, "anchor" colloquially refers to "anchor tags" in HTML documents, which you can reference within a URL with fragment identifiers #someref. The part preceding the :// is called "scheme" in URL/URI vocabulary and we should probably stick with that if we're going to call it anything. (Reference)

Assumes that url must have suffix (e.g., http://example.com/file.txt) to be a file; if no suffix, assumes dir. This is definitely not true of real-world URLs, but maybe is an ok assumption for anything serving files?

I'm not sure I like this. There are often genuine files that don't have file extensions that are usually plain text files, e.g., README, LICENSE, Makefile.

My impression is that, (while it's not absolute) the most common convention and default for many web servers and frameworks is to use a trailing slash explicitly to serve directories.

@pjbull
Copy link
Member Author

pjbull commented Sep 4, 2024

We shouldn't call this "anchor". Anchor has a different meaning in a close enough context that I think this will be confusing.

Hm, yeah, I can see this is potentially confusing, but we're not calling it "anchor" arbitrarily. We're looking for the right analogy to populate the existing .anchor pathlib property. In the pathlib context .anchor is a Path object that is drive + root. We're using it to refer to scheme + netloc (not just the scheme) which reasoning by analogy feels about right. The difference from the cloud providers is that the scheme is the root is the drive (e.g., listing s3:// lists buckets you have access to). IMO docs can cover this source of potential confusion without too much worry, especially since anything referring to an anchor in a url is most often called the "fragment".

use a trailing slash explicitly to serve directories.

Yeah, this was the other default rule I considered—I think you're right it's probably a more reliable default. That is how the python server works (except for the root, which does not redirect to the slash). Also planning for the method to be overridable so people can configure for their particular servers if it is different.

@github-actions github-actions bot temporarily deployed to pull request September 13, 2024 10:39 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 16, 2024 18:58 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 17, 2024 21:55 Inactive
@pjbull pjbull mentioned this pull request Sep 19, 2024
@MattOates
Copy link

MattOates commented Oct 9, 2024

So one thing to be aware of is your code assumes no one would ever do HttpPath("http://username:password@host/path/file.txt") for basic auth.

url.netloc looks like the string "username:password@host" in this instance. Im currently struggling to see how in the Cloudpathlib design I can actually inject the auth from the URL into the client too. Looks more like you would always have to explicitly provide a client which feels not incredible from the user side. Perhaps we could introduce something that for http/sftp/ftp etc user/pass auth has some standard parameters that get passed to the client from the URL if the netloc has an @ in?

@pjbull
Copy link
Member Author

pjbull commented Oct 14, 2024

So one thing to be aware of is your code assumes no one would ever do HttpPath("http://username:password@host/path/file.txt") for basic auth.

@MattOates I guess I was thinking that this just sticks around on that path and any paths derived from it (since we don't mess with netloc) and should "just work." Does it not? Maybe there are just some bugs to clean up (sorry, haven't had a chance to poke at it and confirm/deny).

Looks more like you would always have to explicitly provide a client which feels not incredible from the user side.

I think that we could (1) support env vars for basic auth like most of the cloud providers have for auth, (2) point people towards how to set the default client, or (3) recommend an explicit client.

@jayqi
Copy link
Member

jayqi commented Oct 18, 2024

I guess I was thinking that this just sticks around on that path and any paths derived from it

Does that mean we're going to print out the username and password in plaintext in string representations?

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.

Read from http - httppathlib?
3 participants