[INT-1234] Refactor to use client MountedRepo state #9954
Conversation
emmajsadams
requested review from
a team and
smalyala
and removed request for
a team
| @@ -699,7 +640,7 @@ def _get_file_from_path(self, path: str) -> pfs.File: | |||
| else: | |||
| pach_path = str(Path(*parts[1:])) | |||
|
|
|||
| file_uri = f"{commit_uri}:{pach_path}" | |||
| file_uri = f"{branch.as_uri()}:{pach_path}" | |||
There was a problem hiding this comment.
We can remove commit_uri a few lines above since it's no longer used
There was a problem hiding this comment.
Ah actually I should not have changed this method to add branch. DatumManager should remain the same. I will just revert adding the param for this method.
|
|
||
| return { | ||
| pfs: { | ||
| name: `${mountedRepo.repo.project}_${mountedRepo.repo.name}_${mountedRepo.mountedBranch.name}`, |
There was a problem hiding this comment.
Should the last entry be mountedRepo.mountedBranch?.name
There was a problem hiding this comment.
We check the mountedRepo param earlier in this method for null and return {} so I think this is safe as is.
There was a problem hiding this comment.
Is it possible for mountedRepo to be non-null and mountedBranch to be null?
There was a problem hiding this comment.
@smalyala nope. mountedRepo is non null when the user has selected at least a project/repo. Selecting that will automatically select a branch (usually master). It can only be changed to other projects, repos, branches with no option to manually unselect just a branch.
There was a problem hiding this comment.
Should we pass the branch as part of the request data rather than as a query parameter, since the endpoint path isn't as secure (could be cached, logged, etc.) as the request payload? This is a (rare) concern if a user uses HTTPS requests and considers repo/branch name to be confidential.
Also, can we add some tests to cover cases where the branch argument in the FE request has bad data + missing data (no branch specified)?
Probably a good thing to look into! Should be able to to modify the endpoint to do this.
Will do. |
Hmm so on second thought, I wonder if we should reach out to customer eng on if this is a use case and tackle it another story? We probably want to reevaluate all the paramaters if them possibly being logged as part of a GET request is a concern specifically file name. If the connection between the client and server is HTTPS then the GET parameters are protected in transit so are we worried about the GET url being logged somewhere? Also added the tests! |
|
In general, it's good practice to avoid passing sensitive information with query parameters (i.e. could be logged on server side, stored in browser history, etc.) even if encrypting data with HTTPS. A few searches can provide more comprehensive info and examples than I can :) Yep, I'm good with asking customer eng if this is a potential concern (it's probably not) and tackling it in a separate story if needed. I don't think we need to worry about file name because it's just the repo and branch name that are being passed as query parameters right? |
|
@smalyala sounds good created https://pachyderm.atlassian.net/browse/INT-1257! Is the rest of the PR ready to merge? |
|
Cool, thanks! Last comment here- #9954 (comment) |
This PR refactors the backend to remove server MountedRepo state in favor of client state. It maintains existing behavior, updates all relevant tests, and adds new tests for PollMount.