-
-
Notifications
You must be signed in to change notification settings - Fork 48
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
Split FilePath constructor to guarantee constructor with AmbientAuth #190
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
- Feature Name: Split `FilePath` constructor to guarantee constructor with `AmbientAuth` | ||
- Start Date: 2021-07-02 | ||
- RFC PR: | ||
- Pony Issue: | ||
|
||
# Summary | ||
|
||
It should be without error to construct a `FilePath` when `AmbientAuth` is available as `FilePath` is a pairing between some path string and capabilities on that path. For capabilities, the current constructor uses a union of `(FilePath | AmbientAuth)`. Insufficient capabilities or access allows the first half of this union, `FilePath`, to fail, however the latter half, `AmbientAuth`, can never fail. | ||
|
||
# Motivation | ||
|
||
Beyond making `FilePath` more convenient, this change will allow use cases where a path is guaranteed such as constructing a path to the root directory. Currently, all `FilePath` objects must be constructed within a try-else block. Importantly, `FilePath` does not guarantee its post-construction operations will succeed, however a `FilePath` is merely a combination of some directory path and capabilities. No changes to __using__ a `FilePath` are intended by this change, only construction is affected. | ||
|
||
# Detailed design | ||
|
||
The suggested change is to split the implementation in two around the current match statement with `create` using a new package-level root authority `FileAuth` (created from `AmbientAuth`) and a new constructor called `from` using an existing `FilePath`. | ||
|
||
Current constructor (documentation excluded): | ||
|
||
```pony | ||
new val create( | ||
base: (FilePath | AmbientAuth), | ||
path': String, | ||
caps': FileCaps val = recover val FileCaps .> all() end) | ||
? | ||
=> | ||
caps.union(caps') | ||
|
||
path = match base | ||
| let b: FilePath => | ||
if not b.caps(FileLookup) then | ||
error | ||
end | ||
|
||
let tmp_path = Path.join(b.path, path') | ||
caps.intersect(b.caps) | ||
|
||
if not tmp_path.at(b.path, 0) then | ||
error | ||
end | ||
tmp_path | ||
| let b: AmbientAuth => | ||
Path.abs(path') | ||
end | ||
``` | ||
|
||
Will become: | ||
|
||
```pony | ||
new val create( | ||
base: FileAuth, | ||
path': String, | ||
caps': FileCaps val = recover val FileCaps .> all() end) | ||
=> | ||
caps.union(caps') | ||
|
||
path = Path.abs(path') | ||
``` | ||
|
||
and | ||
|
||
```pony | ||
new val from( | ||
base: FilePath, | ||
path': String, | ||
caps': FileCaps val = recover val FileCaps .> all() end) | ||
? | ||
=> | ||
caps.union(caps') | ||
if not base.caps(FileLookup) then | ||
error | ||
end | ||
|
||
let tmp_path = Path.join(base.path, path') | ||
caps.intersect(base.caps) | ||
|
||
if not tmp_path.at(base.path, 0) then | ||
error | ||
end | ||
path = tmp_path | ||
``` | ||
|
||
Definition of `FileAuth`: | ||
|
||
```pony | ||
primitive FileAuth | ||
new create(from: AmbientAuth) => | ||
None | ||
``` | ||
|
||
This matches the net package design of using a package-level root authority rather than `AmbientAuth` directly. | ||
|
||
# How We Teach This | ||
|
||
Nothing about our teaching should need to change, however this change will allow early tutorials to avoid introducing try-else blocks and partial functions for longer if they so choose. For example, with a separate `FilePath` constructor an early tutorial could construct a `FilePath` to a file containing a simple message in the current directory, construct the necessary `File`, and call `File.read()` or `File.lines()`, none of which would be partial functions after this change -- such a program is just beyond the complexity of the ever ubiquitous "Hello, World" initial lesson. | ||
|
||
Advanced users of Pony, should have little difficulty switching based on the simple statistics from stdlib below. | ||
|
||
- Uses of `FilePath`: 166 | ||
- Uses of `FilePath(...)` from `AmbientAuth`: 37 | ||
- Uses of `_ as FilePath` typing: 19 | ||
- Uses of `FilePath(...)` not from `AmbientAuth`: 29 | ||
- Uses referring to `FilePath` for its type (e.g., within unions): 53 | ||
- Uses excluding the above (mostly documentation): 28 | ||
|
||
# How We Test This | ||
|
||
All unit tests with need to be updated as nearly all existing tests use `AmbientAuth`. At least one additional unit test is recommended to check that serial construction first via `FileAuth` then via `FilePath.from(...)` using the first construction's `FilePath` is error-free when both are attempting to access an identical path. | ||
|
||
Existing unit tests will need to change as the common phrase `let filepath = FilePath(h.env.root as AmbientAuth, path)?` will no longer work. As well, `var tmp_dir: (FilePath | None) = ...` will likely not be needed in as many places as this is a defense against `FilePath` failing within a try statement. | ||
|
||
# Drawbacks | ||
|
||
- Breaks existing code | ||
- Added maintenance cost | ||
|
||
# Alternatives | ||
|
||
Keeping the implementation as it currently is written. There is nothing overtly wrong with the existing implementation, this RFC is a suggestion for making the files package easier to use. | ||
|
||
# Unresolved questions | ||
|
||
Does this have farther reaching affects beyond what is initially seen? Are there current patterns outside of stdlib for managing `FilePath`s? Do we have any need for sub-authorities below `FileAuth`? | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of sub-authorities that might be needed that couldn't be accomplished with just
FilePath
.If we think of sub-authorities in the future, we should be able to add them without a breaking change.
So I'm not worried about this as an unresolved question at this time.