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

Storage repository #1768

Merged
merged 106 commits into from
Jan 18, 2021
Merged

Storage repository #1768

merged 106 commits into from
Jan 18, 2021

Conversation

charlesdaniels
Copy link
Member

@charlesdaniels charlesdaniels commented Jan 11, 2021

charlesdaniels and others added 12 commits December 17, 2020 15:14
The underlying work to provide implementations of these methods has not
been done yet pending discussion, so this will break the build.

This also adds Destroy() as described in #1509, though again no
implementation is defined yet.

This also adds detailed comments to the members of the URI interface, to
make it clear what is expected of URI implementations.
This will be immediately reverted, but if we decide to revive it for
some reason, it might be a good starting point. I started writing this
and then realized Go already has an RFC3986 implementation here:
https://golang.org/pkg/net/url/

I plan to use that unless there is some reason to build out or own
instead.
In retrospect, there is a lot of redundancy In the prose of the given
implementation. I will do a second pass to tidy it up at some point.
Note that I have veered much more towards @toaster 's (Tilo) proposal
than I had originally. My change of heart is documented in the relevant
proposal here:

 https://github.com/fyne-io/fyne/wiki/Proposal%3A-The-Fyne-URI-Philosophy

I have decided not to implement URINotListable on the basis that
URIOperationImpossible already encompasses it's purpose, and
additionally I want to encourage people to use Listable() rather than
just trying to List() and then checking the error.

I decided to move List and Listable() into the Repository interface,
though developers can always throw URIOperatioNotSupported if they are
impossible to implement. I could be convinced to move them into their
own ListableRepository interface to be optionally registered, but to
keep them together (since you cannot have one without the other). This
was discussed a little bit, but I am on the fence.

Since I decided to go with more of Tilo's approach, I did not add Move()
or Duplicate(). If folks have opinions on using these names versus
Rename() and Copy(), please add them on the PR.
Per our meeting today, I am merging the existing work into the new `storage_repository` feature branch so @andydotxyz and I can collaborate more easily. We will open a new PR `storage_repository -> develop` soon.

Despite what the GitHub UI may say, this does **not** affect the approval state of this PR as far as the core team is concerned.
@charlesdaniels
Copy link
Member Author

As of eb6c773 I have stubbed out everything from the proposal. What remains is:

  1. Implementing the TODOs
  2. Implementing the registration system
  3. Implementing the FileRepository
  4. Implementing a new generic URI implementation that supports the updated URI interface.
  5. Re-working driver code for the desktop and mobile drivers.
  6. Tests!

I had to diverge from the proposal in the following ways:

  • I have added a RepositoryForURI method to storage/repository which returns the repository instance registered for a given URI. This was needed because other methods in the repository package need to have access to the registered methods for schemes (e.g. GenericCopy()), but methods in the storage package still need to be able to access these methods as well.
  • I think that we need to be able to return URIRootError from inside of repository. I propose moving it into repository and then changing storage.URIRootError to just be assigned repository.URIRootError, with a marked deprecation. I have not done this, but plan to if nobody has a better idea.

@charlesdaniels
Copy link
Member Author

charlesdaniels commented Jan 12, 2021

Another case where I needed to differ: when we removed ParseURI() from Repository, it seems we also removed it from storage. To the extent of my understanding, it should still be present in storage, so I have re-added it. Otherwise, there is no entry point for obtaining a new URI.

We also did not discuss if we intended to keep or deprecate NewFileURI(). I think this is likely to be a very common-case thing to want to do, so I motion we keep it in.

@charlesdaniels
Copy link
Member Author

I have realized that we never properly discussed what to do with storage.NewURI(). I have tentatively marked it as deprecated, since it cannot report an error.

Most notably, I have renamed RepositoryForURI() to
RegisteredRepository(), to placate the linter.
@AlbinoGeek
Copy link
Contributor

I feel the divergences are acceptable.

If it was added in 1.4 all we can do is deprecate it, I think?

Otherwise, yes, I (as a Developer) one way to Create (New) and one way to Get (Parse) a URI.

So far I'm happy with your approaches / mediations.

@charlesdaniels
Copy link
Member Author

If it was added in 1.4 all we can do is deprecate it, I think?

AFAIK, policy is that it stays in for at least one major release.

So far I'm happy with your approaches / mediations.

Minor note - I changed RepositoryForURI() because it made the linter mad, so it's now RegisteredRepository(). Suggestions welcome.

@andydotxyz
Copy link
Member

NewFileURI is indeed very common. It delegates to NewURI.
I cannot remember what ParseURI erros were expected to be, gven that we are offering basically no guarantees in the URI implementation with regards to its suitability for the repository it relates to...

I don't think we can move NewFileURI to ParseFileURI as that isn't really what it's doing.

@andydotxyz
Copy link
Member

Minor note - I changed RepositoryForURI() because it made the linter mad, so it's now RegisteredRepository(). Suggestions welcome.

What was the error? I don't think RegisteredRepository really describes the action. RepositoryForScheme would be more accurate.

Copy link
Member

@stuartmscott stuartmscott left a comment

Choose a reason for hiding this comment

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

The comment to code ratio is incredible!

Added a few comments of items that are more obvious to me from reading this PR than were from reading the wiki spec.

Great work Charles!

storage/repository/repository.go Show resolved Hide resolved
storage/repository/repository.go Outdated Show resolved Hide resolved
storage/repository/repository.go Show resolved Hide resolved
@charlesdaniels
Copy link
Member Author

charlesdaniels commented Jan 12, 2021

@andydotxyz

I don't think we can move NewFileURI to ParseFileURI as that isn't really what it's doing.

It also handles back-slashes on Windows, which we probably want to keep working for usability even if it's not in the spec.

What was the error? I don't think RegisteredRepository really describes the action. RepositoryForScheme would be more accurate

repository.Repository... "stutters" according to the linter. If you are OK with it and know how to disable linting for that method, I think it was a better name. (edit for posterity, here is a deep link to the exact error)

@stuartmscott
Copy link
Member

What about repository.Lookup(string)?

@charlesdaniels
Copy link
Member Author

Consider testing for infinite recursion in symlinks. Most *mix tools are intelligent enough to give up once they hit a recursive link.

(context)

@AlbinoGeek

I don't think this is a significant risk, since we don't ever recurse, other than I guess in file dialog where the user presumably knows what's going on.

As it stands, FileRepository follows the default simlink handling behavior of the standard library. I trust the Go devs to have non-stupid defaults.

I think this is a good thing to have in mind, but I view this as a path normalization thing, which we agreed to punt to the future.

@charlesdaniels
Copy link
Member Author

Decision on Directory Creation (CreateListable())

We realized that creating directories was impossible with the API as specified. This was discussed in this thread. As a result, we decided to add CreateListable(u fyne.URI) error to the ListableRepository interface.

We also considered creating a new interface (e.g. ListableCreatorRepository), or adding it to WriteableRepository (as it technically is performing a write operation), but decided against both cases.

We note that implementations may return ErrOperationNotSupported in the event that the repository is read-only but is still list-able, e.g. in some hypothetical zipfs:// repository.

The relevant changes were added in fa994f8 and tests added in 03aca12.

@charlesdaniels
Copy link
Member Author

charlesdaniels commented Jan 17, 2021

I suspect you are correct. I knew we would need to do our own parser, or find one, but I had hoped to kick the can down the road. If using ttacon/uri makes the tests pass, I have no issue with using it as an internal-only dependency. At a skim, it looks decently written.

(context)

@andydotxyz

I think your issue should be fixed. As of d95d323 I am using fredbi/uri for URI parsing. However, I am using an ugly hack to get it to correctly handle cases like foo:///some/path, since it thinks the host is empty and throws an error.

I will raise the issue upstream and see what the author of the parser says. RFC396 uses this style of URI as an example explicitly (IETF RFC3986, §1.1 pp. 5).

Edit: issue opened upstream.

andydotxyz
andydotxyz previously approved these changes Jan 17, 2021
Copy link
Member

@stuartmscott stuartmscott left a comment

Choose a reason for hiding this comment

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

PR is looking good, only one unresolved comment remaining

@charlesdaniels
Copy link
Member Author

@stuartmscott I think this is the only remaining comment thread that is open. It is solved to my satisfaction unless you had something else you wanted to discuss on that front?

Copy link
Member

@stuartmscott stuartmscott left a comment

Choose a reason for hiding this comment

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

internal/repository/file.go Outdated Show resolved Hide resolved
storage/repository/repository.go Outdated Show resolved Hide resolved
internal/repository/memory.go Outdated Show resolved Hide resolved
charlesdaniels and others added 3 commits January 17, 2021 20:43
Co-authored-by: Stuart Scott <stuart.murray.scott@gmail.com>
Copy link
Member

@stuartmscott stuartmscott left a comment

Choose a reason for hiding this comment

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

Missed a couple, but otherwise this PR looks good to go and should be able to merge tonight

internal/repository/file.go Outdated Show resolved Hide resolved
storage/repository/generic.go Outdated Show resolved Hide resolved
@stuartmscott
Copy link
Member

stuartmscott commented Jan 18, 2021

I think the widget.Form failures are flakey, can you try re-run them?

--- FAIL: TestForm_Hints (0.21s)
    util.go:47: 
        	Error Trace:	util.go:47
        	            				test.go:38
        	            				form_test.go:151
        	Error:      	Not equal: 
        	Test:       	TestForm_Hints
        	Messages:   	Image did not match master. Actual image written to file://D:\a\fyne\fyne\widget\testdata\failed\form\hint_initial.png.
    util.go:47: 
        	Error Trace:	util.go:47
        	            				test.go:38
        	            				form_test.go:154
        	Error:      	Not equal: 
        	Test:       	TestForm_Hints
        	Messages:   	Image did not match master. Actual image written to file://D:\a\fyne\fyne\widget\testdata\failed\form\hint_invalid.png.
    util.go:47: 
        	Error Trace:	util.go:47
        	            				test.go:38
        	            				form_test.go:157
        	Error:      	Not equal: 
        	Test:       	TestForm_Hints
        	Messages:   	Image did not match master. Actual image written to file://D:\a\fyne\fyne\widget\testdata\failed\form\hint_valid.png.

@charlesdaniels
Copy link
Member Author

You OK if I merge @stuartmscott ?

@stuartmscott
Copy link
Member

Go ahead!

@charlesdaniels charlesdaniels merged commit 7abf690 into develop Jan 18, 2021

// There was no repository registered, or it did not provide a parser

// Ugly hack to work around fredbi/uri. Technically, something like
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: forgot to update this comment, but actually foo:///bar/baz is totally valid and should give us /bar/baz as the path. This is documented in the discussion thread on #1768.

@andydotxyz andydotxyz deleted the storage_repository branch April 7, 2021 14:07
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.

5 participants