Skip to content

Proposal: The Fyne URI Philosophy

Jump to bottom
Andy Williams edited this page Apr 2, 2024 · 49 revisions

Note

This document is archived as it refers to a design process.

Cross-References

Introduction

Following the discussion on 2020-12-18 relating to #1509 and #1665, several decisions have been made relating to what methods should be part of the URI interface, and which should be helper methods in the storage package. This proposal summarizes the decisions made, the rationale, and invites further discussion by the Fyne community. Please add any discussion to the Discussion section below.

Sign Off

This proposal is considered final and is slated to be implemented. Core developers who have signed off on this decision should list themselves below:

  • Charles Daniels - @charlesdaniels
  • Andy Williams - @andydotxyz

Revised Proposal

Based on the 2021-01-08 meeting, this is the final revised version of this proposal. A new discussion section has been created for any final revisions or comments.

Proposed API Changes

package repository

  • type Repository interface
    • Exists(fyne.URI) (bool, error)
    • Reader(fyne.URI) (fyne.URIReadCloser, error)
    • CanRead(fyne.URI) (bool, error)
    • Destroy()
  • type WriteableRepository interface
    • Repository
    • Writer(fyne.URI) (fyne.URIWriteCloser, error)
    • CanWrite(fyne.URI) (bool, error)
    • Delete(fyne.URI) error
  • type HierarchicalRepository interface
    • Repository
    • Child(fyne.URI) (fyne.URI, error)
    • Parent(fyne.URI) (fyne.URI, error)
  • type ListableRepository interface
    • Repository
    • List(fyne.URI) ([]URI, error)
    • CanList(fyne.URI) (bool, error)
  • type CopyableRepository interface
    • Repository
    • Copy(fyne.URI, fyne.URI) error
  • type MoveableRepository interface
    • Repository
    • Move(fyne.URI, fyne.URI) error
  • func Register(scheme string, repository Repository)
  • func GenericParent(u fyne.URI) (u fyne.URI, error)
  • func GenericChild(u fyne.URI, component string) (u fyne.URI, error)
  • func GenericCopy(source fyne.URI, destination fyne.URI) error
  • func GenericMove(source fyne.URI, destination fyne.URI) error
  • OperationNotSupportedError

package storage

  • func Exists(u fyne.URI) (bool, error)
  • func Reader(u fyne.URI) (fyne.URIReadCloser, error)
  • func CanRead(u fyne.URI) (bool, error)
  • func Writer(u fyne.URI) (fyne.URIWriteCloser, error)
  • func CanWrite(u fyne.URI) (bool, error)
  • func Delete(u fyne.URI) error
  • func Child(u fyne.URI, component string) (fyne.URI, error)
  • func Parent(u fyne.URI) (fyne.URI, error)
  • func List(u fyne.URI)
  • func CanList(u fyne.URI) (bool, error)
  • func Copy(source fyne.URI, destination fyne.URI) error
  • func Move(source fyne.URI, destination fyne.URI) error

package fyne

  • type URI interface
    • Extension() string
    • Name() string
    • MimeType() string
    • Scheme() string
    • Authority() string
    • Path() string
    • Query() string
    • Fragment() string

Commentary

This approach ended up looking a lot like Tilo's original suggestion. We went with this style because it makes it easier to do additive API changes in a backwards compatible way later. If all of the new methods in storage were instead in the URI interface, we would need to break the public API any time we wanted to add a new method.

Using interfaces to implement all of the different types of Repository buys us three things. First, it means we can expand what is possible with repositories in the future by adding new interfaces that embed Repository. Second, it simplifies registration, as the Register() function can internally use type assertions to determine which operations are implemented. Finally, it allows us to force certain methods to be implemented together, such as CanList and List, and also guarantees that optional methods like these cannot be registered without a base repository.

Repository implementations must implement all interfaces for methods they wish to support, and any attempts to call higher-order functions in the storage will fail with OperationNotSupportedError errors if the underlying interfaces are not implemented. To make this easier for Repository developers, Generic...() methods are provided in the repository package which implement common-case but unoptimized versions of many methods.

The OperationNotSupportedError error indicates that the operation is not supported for any URI of the given scheme. The Repository implementation may also pass along it's own error types which indicate more specific failures.

The Destroy() function is introduced into Repository in this final version, because the Register() function overwrites any previously registered repository for a given scheme. For some repositories, it may be necessary to perform clean-up in this event. Repositories that don't need to do this can simply stub out this method.

We note that there may be cases where a single scheme may need different repositories. To this end, we propose that in the future, a RepositoryRouter can be introduced, which implements the Repository interface externally, but then allows other Repository instances to be registered with it internally, distributing API calls to them as appropriate based on some criteria. This is out of scope for this particular proposal and the work being done in the context of 2.X however.

We note that the Repository interface is not intended to be used by the typical Fyne developer, only those who are implementing new storage back-ends. For the vast majority of people who will use the storage package, Repository can remain opaque and untouched.

Optimization & Caching

The Repository interface is specifically intended to enable optimized operations for storage back-ends, and to give them the necessary hooks to perform their own caching. For example, an ssh: back-end might choose to implement the optional CopyableRepository to allow it to run cp on the remote system. If it wished to use a generic version based on Reader and Writer, it could use the repository.GenericCopy() helper method, which would forego any such optimizations.

Repository implementations are encouraged to implement their own caching where it is appropriate. For example, they might choose to run a goroutine after Register() has been called to maintain state, handle authentication, etc.

Authentication & State

Repository is an interface, meaning it must have some kind of lower level struct to implement it. It is appropriate and encouraged for this struct to handle matters of authentication, such as storing an authentication token. For the time being, such structs can and should implement their own methods for interacting with implementation details - "authentication" varies between services, and we do not want to define a specific authentication API until it is more clear in what contexts it might be used. If this is done in the future, it will be an additive change, with an AuthenticatedRepository interface or something to that effect being added.

Change History

  • Fri Jan 8 11:59:06 PM EST 2021 - Charles - Updated the proposal to break out HierarchicalURI, ReadableURI, and WriteableURI per Slack discussion.

  • Mon Jan 11 02:16:40 PM EST 2021 - Charles - move ReadableRepository and HierarchicalRepository back into the Repository interface.

  • Mon Jan 11 02:40:26 PM EST 2021 - Charles - pull out HierarchicalRepository again, remove ParseURI, move Delete() into WriteableRepository.

  • Mon Jan 11 02:46:24 PM EST 2021 - Charles - Add storage.CanWrite and storage.CanRead.

  • Mon Jan 11 02:47:27 PM EST 2021 - Charles - Change ReaderFrom and WriterTo to Reader and Writer.

  • Mon Jan 11 02:51:22 PM EST 2021 - Charles - s/URIOperationGenericError/URIRequestFallbackError/g

  • Mon Jan 11 04:19:37 PM EST 2021 - Charles - Split repository package and add Generic methods.

  • Mon Jan 11 04:22:39 PM EST 2021 - Charles - Move errors to storage/repository.

  • Mon Jan 11 04:43:01 PM EST 2021 - Charles - remove URIRequestFallbackError and URIOperationImpossible

  • Mon Jan 11 04:55:03 PM EST 2021 - Charles - s/URIOperationNotSupportedError/RepositoryOperationNotSupportedError

  • Mon Jan 11 04:56:17 PM EST 2021 - Charles - s/RepositoryOperationNotSupportedError/OperationNotSupportedError

Discussion

Mon Jan 11 02:17:01 PM EST 2021 / Charles

I have moved the ReadableRepository and HierarchicalRepository methods back into Repository. There appeared to be a lot of waffling in our discussion in Slack. I don't think there was much strong conviction for one direction or another. I feel strongly that these methods belong in Repository, so in the absence of a good technical reason to do something else, this is the decision I'm making, since we have to make some decision on this front.

I am keeping WriteableRepository separate, since I think there are a decent number of potential use cases for repositories that don't implement write support.

I have added CanRead and CanWrite methods as a surface area for performance optimization for implementations, since it may be cheaper compared to trying to do it and checking for an error in certain contexts.

I think there was some discussion on Slack about adding stuff into Repository to make registered repositories be able to contribute things to the file dialog "favorites" section. I think this is a good idea, but is out of scope for 2.0.0. We can revisit this later, possibly by adding a new interface to add this functionality.

Mon Jan 11 02:41:10 PM EST 2021 / Charles

After some discussion with Stuart, I realized my previous changes didn't quite line up with what we have planned. HierarchicalRepository has been pulled back out, on the basis that in the common case, people will want to use the default implementation rather than writing these methods themselves.

ParseURI has been removed, since we expect that the default implementation using net/url and the storage.ParseURI end point will satisfy most foreseeable use cases. If necessary in the future, we can re-add a ParseableRepository interface.

I have moved Delete() into WriteableRepository because we had discussed that in Slack, and I forgot to make the change in my previous edit. It seems fairly obvious that if the baseline Repository only does read-only things, Delete() does not fit in.

Previous Proposal & Discussion

Decision Reached 2020-12-18

The URI and ListableURI interfaces should fully encapsulate all implementation functions required to interact with both the URI itself, as well as the resource that it references. Helper methods placed in the storage package will be used only to handle meta-level considerations, such as passing data between multiple URI implementations. A future "registry" system will be used for the purpose of allowing specific URI implementations to be associated with URI schemes for the purpose of hydrating URIs from strings.

Note however that it is possible and allowed for a specific URI implementation to handle multiple schemes. For example, a URI of a given scheme might return a URI of a different scheme as it's parent. However, it is obviously necessary that the URI implementation must internally define or otherwise be aware of all other URI schemes it needs to interact with in such cases. It is expected that this will come up rarely.

This means that nearly all current URI-related functionality will be rolled into either the URI or ListableURI implementations. Methods including Parent, Child, ReaderFrom, and WriterTo will be folded into the interface implementations (as part of #1665). Methods that cannot be supported by the URI scheme will simply throw errors (and new error types will be created to allow this to be checked for in particular). We note that it may be possible to read or write data to a ListableURI, even though this may seem unintuitive since it is normally impossible to obtain a reader or writer for a filesystem directory.

The Parent of any URI must definitionally be listable, therefore Parent should return a ListableURI. Taken the other way around, it follows that only a ListableURI can have a Child(). Going forward, Driver.ListerForURI will be removed, and listing a URI will cause the produced list of children to be of the ListableURI type if they are listable (ListerForURI will become a utility method that performs a type conversion, to avoid breaking the API).

We further agreed that we should lean towards having more API endpoints to expose more semantic information to the underlying URI optimization. For example a Rename operation on a remote filesystem gives the implementation the opportunity to perform the rename on the remote end.

Proposed API Structure

Changed items are marked with a ! symbol.

URI

  • Extension() string
  • Name() string
  • MimeType() string
  • Scheme() string
  • ! Parent() (ListableURI, error)
  • ! ReaderFrom() (URIReadCloser, error)
  • ! WriterTo() (URIWriteCloser, error)
  • ! Copy(URI) error
    • This method should copy the referenced resource to some destination URI without modifying the original
  • ! Rename(URI) error
    • This method should rename the reference resource to some destination URI, deleting or otherwise overwriting the referenced URI if it exists already
  • ! Delete(URI) error
    • This method should delete, remove, or otherwise destroy the referenced resource

ListableURI

  • URI
  • ! List() ([]URI, error)
    • The method signature remains unchanged, but this method will now ensure that returned children are of type ListableURI if appropriate, so they can be cast back to ListableURI using a normal type assertion.
  • ! Child(string) (URI, error)
    • This method should instantiate a new URI nested below the referenced resource. For example, creating a child of a directory. Again, the returned child should be of type ListableURI if appropriate so it can be cast back to ListableURI using a normal type assertion.

Storage

  • ! Move(URI, URI) error
    • This method reads the contents of it's first operand, writes the read data to the second operand, and then deletes the first operand. This is similar to URI.Rename(), but can work across URI schemes that do not know about each other.
  • ! Duplicate(URI, URI) error
    • This method works identically to Move(), but does not delete it's first operand. It is the counterpart to URI.Copy().
  • ! RegisterURIImplementation(string, URIImplementation)
    • This method registers a URI implementation associated with a string scheme.
  • ! NewURI(string) (URI, error)
    • This method uses an internal registry to call the appropriate NewURI() method based on the scheme of the string URI.

! URIImplementation

  • ! NewURI(string) (URI, error)
    • This method should create a new URI backed by the underlying URI implementation. If the URI is listable, it should be created as a ListableURI and returned as a URI, so it can later be upcast if needed.

New error types:

  • URIOperationNotSupported - the requested URI or ListableURI API endpoint is not supported by the underlying implementation. For example, attempting to call ReaderFrom() on a ListableURI instance representing a filesystem directory would fail with this error.
  • URIOperationImpossible - the requested operation is supported by the underlying implementation, but violates some constraint internal to that implementation. For example, attempting to create an unrepresentable state on an LDAP server might fail with this error.

Rationale

There was considerable dialog in the 2020-12-18 meeting about the tension between what should be accomplished via registration (having the developer register a callback function associated with a scheme), versus what can be accomplished by adding methods to interfaces. At various points, we tried to divide these ideas up along various lines, such as operations done on the URI, versus operations done on the resource that the URI references. The ultimate conclusion was that the vast majority of Fyne's users (developers) will be using URIs but not implementing them, and on that basis we should prioritize creating the easiest to use API, and that moving as much functionality as possible into the URI/ListableURI interfaces would make a more ergonomic API.

This decision was made with the knowledge in mind that:

  • The basis on which functionality is moved into the URI/ListableURI interface is arbitrary and based on a subjective notion of what makes an API pleasant to use.
  • This will make it more difficult for people to implement new URIs.

Edit History

  • 2020-12-18 - Charles - Initial Version
  • 2020-12-18 - Stuart - Proposals and Typo Fixes

Discussion

Andy: This seems to capture our discussion well. NewURI probably does not need the ! next to it, as it's broadly the same definition as before (though it will look up a registry too).

Andy: I wonder if URI.ReaderFrom and WriterTo might be nicer as Reader and Writer names? Just a thought.

Stuart: Agreed

Andy: Also should this cover the deprecated items for removal, or is that covered elsewhere do you think?

Stuart: I don't see a huge upside to separating them

Stuart: Could Duplicate be called Clone?

Stuart: Could RegisterURIImplementation / URIImplementation be shortened?

Stuart: URIOperationImpossible sounds like it is outside the realm of possibility rather than something was attempted and did not succeed. How about URIOperationFailed, or just return the underlying error?

Stuart: Will there still be a storage.NewFileURI utility to avoid all the duplication of storage.NewURI("file://"+path) in the common case of file handling?

Charles:

Stuart: Could Duplicate be called Clone?

Fine with me.

Stuart: Could RegisterURIImplementation / URIImplementation be shortened?

Does anyone have a suggestion here? Perhaps Handler instead of Implementation?

Stuart: URIOperationImpossible sounds like it is outside the realm of possibility rather than something was attempted and did not succeed. How about URIOperationFailed, or just return the underlying error?

Where appropriate, the underlying error should be returned. The idea with URIOperationImpossible was that some of the operations defined by the API may not make sense in the context of whatever resource the URI represents, and there isn't an underlying error that makes sense. It probably won't come up that much though, so we can drop it and re-add later if needed.

Stuart: Will there still be a storage.NewFileURI utility to avoid all the duplication of storage.NewURI("file://"+path) in the common case of file handling?

I'm not opposed. Realistically dealing with local files is going to be the happy path and the common case.

Tilo:

Note: Unfortunately I missed the chat about this all. Therefore I might now have inappropriate objections that might vanish after I have a better understanding.

I think that the URI interface offers too much functionality. IMO an URI and a resource are two different things and the URI should only be used to reference a resource. Honestly, I can’t even see the need of an interface for URI at all. Why is one implementation not sufficient for the functionality that the current URI interface provides?

Regarding access (reader, writer) to the resource: I think, this is something a repository should provide. It would take an URI as key and then provide access to the resource for this URI. It also would know which kind of URIs (schemes) it supports.

Regarding high-level convenience access (copy, move, delete): These should be limited to the storage package methods. I cannot see any advantage of src.Copy(dest) over storage.Copy(src, dest). It just bloats the interface.

The registry should not be for URIs but for repositories, e.g. one could register an S3Repository for the s3 scheme which provides access to S3 objects. The registration could either be done via RegisterRepostory(repo, scheme) or RegisterRepository(repo) where the repository provides the supported schemes. I think both would have their use-cases. While the latter is more convenient, the former allows better control, especially when there are more than one repository for a scheme around.

A repository would provide access to resources (reader, writer, rename/move (atomic), delete). I think the whole listing stuff (parent/children) would also belong to the repository.

Charles:

The points Tilo raised were indeed some of the things that came up in discussion. Unfortunately, I think my notes didn't capture things at a granular enough level...

I think that the URI interface offers too much functionality. IMO an URI and a resource are two different things and the URI should only be used to reference a resource. Honestly, I can’t even see the need of an interface for URI at all. Why is one implementation not sufficient for the functionality that the current URI interface provides?

The duality between a resource versus a reference to a resource is something we groused about for some time. The proposed implementation is not very philosophically pure in this regard. I think the simplest way to put it is that the URI interface is now really a way of mediating access to an implementation of all of the stuff you can do on a resource of a given type. It handles far more functionality than is required to simply manipulate a URI string.

On closer examination, we found that many seemingly simple concepts like Parent(), Child(), and so on are in fact not so simple, and may require functionality specific to a given URI scheme. By moving these types of methods into the URI interface, it makes it possible for the individual implementation to define the behavior they need.

Regarding access (reader, writer) to the resource: I think, this is something a repository should provide. It would take an URI as key and then provide access to the resource for this URI. It also would know which kind of URIs (schemes) it supports.

I think that is more or less equivalent, but with more steps/complexity. The notion with moving ReaderTo and WriterFrom into the URI interface is that we no longer have to maintain such a repository, and also allows moving that functionality out of the driver (which a registry based approach would also solve). Ultimately, the purpose of a URI is going to be to either read or write to it (or perform some higher-order operation that boils down to reads and writes). If you will grant that the URI interface handles functionality relevant to both the reference and to that-which-is-referenced, I think you will agree that mediating read and write access is appropriate to place here.

Regarding high-level convenience access (copy, move, delete): These should be limited to the storage package methods. I cannot see any advantage of src.Copy(dest) over storage.Copy(src, dest). It just bloats the interface.

Irrespective of the previous points, I must disagree on this strongly. What constitutes a Copy, Move, Delete, etc. will vary by the underlying implementation. Performing a copy on an ssh:// by copying all of the data to the local end, then pushing it back up to the remote is clearly the wrong thing to do here. This was the rationale behind having Copy() versus Duplicate(), and Rename() versus Move(). The versions in the URI interface give the implementation the opportunity to do something more optimal (such as trigger an operation on a remote server, or leveraging an OS level API).

This could also be solved by using a registry that allows URI implementors to register callbacks for these things a-la-cart. I think we (the folks on the call) all agreed that this would produce an un-ergonomic and complicated API.

For clarity to future readers: that I think what you (Tilo) is calling a repository, we are calling a registry or registration in my notes and earlier in this article.

Tilo:

I’m still not convinced that a rich URI is the way to go.

First regarding “repository”: I called it that way because this is what it is called in the Onion Architecture (which you probably already knew). In the end it does not matter if you call it repository, database, storage or whatever. I will stick to repository. And I don’t think that you meant this with “registry”. In my understanding a registry is basically some kind of mapping (like URI scheme to repository).

On closer examination, we found that many seemingly simple concepts like Parent(), Child(), and so on are in fact not so simple, and may require functionality specific to a given URI scheme. By moving these types of methods into the URI interface, it makes it possible for the individual implementation to define the behavior they need.

Parent() and Child() (or Children()) are not a property or concept of URI at all. They are a properties or relations that the resource might have. The resource is managed by the repository and therefore the hierarchical access to resources has to be implemented there.

I think that is more or less equivalent, but with more steps/complexity.

Yes, and no. Yes, it is equivalent. I cannot see where there are more steps involved. No, it is not more complex, only more obvious.

The notion with moving ReaderTo and WriterFrom into the URI interface is that we no longer have to maintain such a repository […].

That’s not really an advantage because you only move the implementation into the URI (where it does not belong).

Ultimately, the purpose of a URI is going to be to either read or write to it (or perform some higher-order operation that boils down to reads and writes).

I don’t think so. The ultimate purpose of a URI is already encoded in its name: “Resource Identifier”. It should identify a resource. The ultimate purpose of that resource is to be accessed in some way. But this access is not performed through the URI but through some repository.

If you will grant that the URI interface handles functionality relevant to both the reference and to that-which-is-referenced, I think you will agree that mediating read and write access is appropriate to place here.

I won’t grant that :). I think the URI should be restricted to what it is: an identifier. The resource access should be done by repositories.

Irrespective of the previous points, I must disagree on this strongly. What constitutes a Copy, Move, Delete, etc. will vary by the underlying implementation. Performing a copy on an ssh:// by copying all of the data to the local end, then pushing it back up to the remote is clearly the wrong thing to do here. This was the rationale behind having Copy() versus Duplicate(), and Rename() versus Move(). The versions in the URI interface give the implementation the opportunity to do something more optimal (such as trigger an operation on a remote server, or leveraging an OS level API).

I cannot see where a repository (let’s say an S3 repository or an SSH repository) cannot perform an optimized (server-side) copy when .Copy(dest, src) is called. Why do you think that only an implementation in a specific URI could do that?

I still think it is better to implement a repository and register it for one or multiple URI schemes instead of bloating the interface of something that is meant to be an identifier.

Andy:

As far as I can see these are conceptually similar. One proposal is URI for identification and access, the other is that URI is identification and Repositiory handles access.

In this case the former would support shorthand like:

read := storage.NewURI(uriString).ReadFrom()

However if we expose the repository separately then it becomes

uri := storage.NewURI(uriString)
read := storage.RepositoryFor(uri.Scheme()).ReadFrom(uri)

I hope that captures the discussion. Both require a repository / registry, but one is providing syntactic sugar over the look-up. In the implementation it will not be so straight forward, but from a users point of view this is the difference.

My preference is to hide the details of the repository from day-to-day usage, though registering custom providers would still need to be a public API.

Tilo:

Good summary, Andy.

I don’t say there should not be convenience methods. I only say they should not be part of the URI interface:

uri := storage.NewURI(uriString)
read := storage.ReadFrom(uri)

Or even:

read := storage.ReadFromURIString(uriString)

Plus, I’m still not convinced that we need an URI interface at all. One could argue that special URI implementations (like MailURI, S3URI, HTTPSURI, FileURI etc.) allow special validation/parsing implementation. But also in this case, I think that this should not be part of the URI but of the repository that uses it as key. The repository knows exactly which URI values are valid. The URI implementation should not implement more than the URI RFC (3986 AFAIK).

Andy

We are talking 15 - 20 methods to handle all the functionality of a storage provider - that feels cluttered if they are all package level definitions. We could move it to a Repository type that the user can look up like storage.RepositoryFor(uri.Scheme()) but then you run the risk of the following, which would be really bad:

u := storage.NewFileURI("/my/path")
r := storage.RepositoryFor(u.Scheme())
u = storage.NewURI("https://google.com/favicon.ico")
r.ReadFrom(u) // this either does nothing or goes boom...

Andy

Another thought. We could keep the repository provider and URI completely seperate whilst delivering the same convenience.

type StorageRepository interface // would implement the required functionality
type URI interface // declares the helpers as with the convenience focused design

type uriImpl struct {// would implement the core URI logic, plus helpers like the following
    res string
}

func (u URI) Read() (fyne.URIReadCloser, error) {
    repo := repoForScheme(u.Scheme())
    if repo == nil {
        return nil, errUnsupportedScheme
    }

    return repo.Read(u)
}

This should provide a separation along with high levels of convenience. Developers looking to extend the functionality would do something like:

storage.AddStorageProvider(scheme string, repo StorageRepository)

Thu Dec 31 02:32:50 PM EST 2020 / Charles

Based on discussions on Slack and in this proposal, here is a new API, which I hope will please everyone (or at least be OK for everyone). Note that this is aspirational - if everyone agrees that this is what we should shoot for, we can then go back and consider how to go from the 1.4.X URI implementation to this.

type rfc3986 struct

  • Our internal implementation of RFC3986.
  • We can also consider either exporting this, or even placing it in an external project.
  • In my view, we shouldn't be directly handling this type on API endpoints, but only using it internally.
  • func (rfc3986) Scheme() string - returns the RFC3986 scheme for the URI
  • func (rfc3986) Authority() string - returns the RFC3986 authority for the URI
  • func (rfc3986) Path() string - returns the RFC3986 path for the URI
  • func (rfc3986) Query() string - returns the RFC3986 query for the URI
  • func (rfc3986) Fragment() string - returns the RFC3986 fragment for the URI
  • Components that are empty will be returned as empty-strings.
  • We may wish to make somethings like Authority() also return rich types, for example the authority might need it's own type to allow easily parsing the user, port, domain, etc. However, it might be better to leave it to the storage repository to deal with/parse.

type URI struct

  • Our implementation of URIs in a developer-facing way, internally backed by rfc3986, possibly exposing those functions.
    • Repository implementations will need access to these functions, so we must expose them one way or the other (either by wrapping them, or making rfc3986 exported and exporting it's member in the URI implementation. Or by some other means.
  • Internally, these methods take the Scheme() of the underlying RFC3986-URI and look up an appropriate handler method in the relevant repository, or fail-over to a generic or primitive version if one is not available.
  • Unless otherwise noted, these methods all work as previously discussed.
  • My rationale for including the URI-related operations in this type rather than in the storage package is because these are indeed all operations that act on a specific URI. I believe that by separating out RFC3986 as it's own thing, separation of concerns between "dumb" string URIs and operations done on a URI is achieved.
  • By making this a struct rather than an interface, we can guarantee we are the only implementation, and can look up the underlying operations appropriately based on the repository system
  • My rationale for removing ListableURI is that it seems like a weird distinction in retrospect. It may require operations in the repository to determine. It may change at runtime. Readable/writeable URIs are also not separated in this way. From a performance perspective, I think there is no reason to prefer a type assertion.
  • func (URI) Extension() string
  • func (URI) Name() string
  • func (URI) MimeType() string
  • func (URI) Scheme() string
  • func (URI) Parent() (URI, error)
  • func (URI) ReaderFrom() (URIReadCloser, error)
  • func (URI) WriterTo (URIWriteCloser, error)
  • func (URI) Copy(URI) error
  • func (URI) Rename(URI) error
  • func (URI) Delete(URI) error
  • func (URI) Listable() bool, error
    • Check if the URI can be listed, can fail if this involves an operation in the underlying repository.
  • func (URI) List() ([]URI, error)
  • func (URI) Child(string) (URI, error)

type Repository interface

  • This interface allows for outside developers to implement new URI-backends.
  • The methods in this interface return functions, rather than directly implementing the functions. This means that an implementation can choose not to implement a function.
  • The second return, the Boolean, is true if the storage package should fail-over to using generic, internal versions of un-implemented functions. If the bool options is omitted, it means that generic functions will be used unconditionally if none is defined (e.g. for mandatory functions).
  • Maybe we want to use names like FooImpl instead of just Foo?
  • This approach makes it easy to define new URIs, especially if you are on the happy-path, you can rely on built-in/generic methods by simply returning nil,true for functions you don't want to implement. For example, if you have ReaderFrom() and WriterTo(), but not Copy(), storage can transparently fail over to doing this the "dumb" way.
  • func NewURI(string) (URI, error)
    • For obvious reasons, you must define a way to create a new URI.
  • func Extension() func (URI) () string
  • func Name() func (URI) () string
  • func MimeName() func (URI) () string
  • func Scheme() func (URI) () string
  • func Parent() (func (URI) () (URI, error), bool)
  • func ReaderFrom() (func (URI) () (URIReadCloser, error), bool)
  • func WriterTo() (func (URI) () (URIWriteCloser, error), bool)
  • func Copy() (func (URI) () error, bool)
  • func Rename() (func (URI) () error, bool))
  • func Delete() (func (URI) () error, bool))
  • func Listable() (func (URI) () (bool, error), bool)
  • func List() (func (URI) () ([]URI, error), bool)
  • func Child() (func (URI) (string) (URI, error), bool)

package storage

  • Move(URI, URI) error
  • Duplicate(URI, URI) error
  • RegisterRepository(string, Repository)
  • NewURI(string) (URI, error)

New error types:

  • URIOperationNotSupported - the requested URI API endpoint is not supported by the underlying implementation. For example, attempting to call ReaderFrom() on a URI instance representing a filesystem directory would fail with this error.
  • URIOperationImpossible - the requested operation is supported by the underlying implementation, but violates some constraint internal to that implementation. For example, attempting to create an unrepresentable state on an LDAP server might fail with this error.
  • URINotListable - if List() is called on a URi which is not listable.

Thu Dec 31 04:21:19 PM EST 2020 / Charles

Some additional comments pulled over from slack:

  • Name(), Scheme(), and Extension() do not belong to the Repository interface since they are simply URI string parsing ; they should not be implemented there (leaving the above unmodified for posterity).
  • ListableURI is really just an implicit form of caching whether or not something is listable. A better argument than used above for moving List() to URI and adding IsListable() is that it allows caching of this information to be handled explicitly.
  • On that note, some performance optimizations (e.g. avoiding calling stat() on files on a remote machine to check IsListable() by caching this information) require caching. A func (URI) Refresh() should be added to the URI struct (and a corresponding function call in the Repository interface) as a way of explicitly telling the repository it needs to flush any caches relating to the URI.
  • We may want or need to add some kind of configuration method (e.g. a KVP store) to allow tuning repositories (e.g. configuring caching behavior, or passing flags to some underlying program or driver). This change can be made additively later, so is out-of-scope for this proposal.

Fri Jan 1 10:47:17 AM UTC 2021 / Tilo

The new proposal is far better but I still have some objections of different severity and also questions and proposals.

  1. Questions

    1. I had parsing errors when trying to understand some of the function definitions. For instance in func ReaderFrom() (func (URI) () (URIReadCloser, error), bool): what does the (URI) part mean?
    2. What is the purpose of Repository.MimeName()?

  2. Weak objections (I’m against but won’t insist)

    1. Moving the URI into a type instead of an interface reduces the severity of my objections against its convenience methods. I still think it is wrong to have them there but at least we do not enforce others to implement them. The list of methods I don’t would introduce in URI is:

      • Child
      • Copy
      • Delete
      • List
      • Parent
      • ReaderFrom
      • Rename
      • WriterTo

    2. I cannot see where the separation of URI and rfc3986 is an improvement. It does not affect my opinion about the URI being a dump identifier.

  3. Strong objections (I’m against and won’t back this)

    1. While I won’t insist on removing the convenience methods from the URI, I still want to have them in the storage package:
      • func Child(URI, string) (URI, error)
      • func Copy(URI, URI) error
      • func Delete(URI) error
      • func List(URI) ([]URI, error)
      • func Listable(URI) (bool, error)
      • func Parent(URI) (URI, error)
      • func ReaderFrom(URI) (URIReadCloser, error)
      • func Rename(URI, URI) error
      • func WriterTo(URI) (URIWriteCloser, error)

  4. Proposals

    1. Rename NewURI to ParseURI. The latter make more explicit that there is parsing involved. Also, I don’t think that a constructor (method starting with New) should return an error.

P.S.: Okay, turned out it was only one proposal and one strong objection but I won’t reformat this now :).

Fri Jan 1 02:28:13 PM EST 2021 / Charles

In response to Tilo's above points:

  • 1.1 - func ReaderFrom() (func (URI) () (URIReadCloser, error), bool) means a function that returns a pointer-receiver to a URI (should be URI* not URI in retrospect), which takes no args, and returns a URIReadCloser, error, followed by a boolean flag.
  • 1.2 - typo, should be MimeType(), I left this in since it was already there.
  • 2.2 - Maybe we should use a name other than URI. The notion was that it's separating the concerns about "what the RFC says", versus "what we have built on top of the RFC". I will advocate strongly for having this distinction internally (unexported), and weakly for exporting it.
  • 3.1 - I am amenable to this. I don't think this adds much complexity either way.
  • 4.1 - I like this idea, agree.

Here is something I would propose, but I would like Andy and Damon to weigh in also:

  • Keep methods already on URI/ListableURI for backwards-compat, but mark as deprecated.
  • Expose the methods I have discussed related to the RFC (e.g. Authority()) on URI (or export the RFC3986 member).
  • Introduce the methods Tilo has discussed in the storage package only for now (not in the URI type).
  • As we move along, we can add those methods to the URI or some other helper type in an additive way if we feel it is necessary. This could be done without breaking backwards compat if the endpoints in the storage package are maintained.

Sat Jan 2 18:12 UTC 2021 / Stuart

Thinking more about the longer term functionality, it becomes clearer to me that an abstraction above individual URIs such as Repository is a good idea. Consider these additions to File Dialog;

  • A Repository could provide a set of URIs to display in the Favourites section
  • A Repository could provide optimized Searching, Filtering, and Sorting
  • A Repository may need to handle identity/account management for remote file systems or cloud storage providers

Andy 4 Jan:

Charles:

A func (URI) Refresh() should be added to the URI struct (and a corresponding function call in the Repository interface) as a way of explicitly telling the repository it needs to flush any caches relating to the URI.

This makes the assumption that the consumer of a URI or repository knows more about the external state and therefore need to refresh than the repository does. Unless you can show a real-world use-case where this is true I think that cache management should be handled internally.

As we move along, we can add those methods to the URI...

Unfortunately not - the URI is a public interface, we cannot just add to it outside of a breaking release. That is basically the whole reason to settle this design ASAP, so that we can settle on a definition of URI that will need to stay for the forseeable future.

Tilo:

While I won’t insist on removing the convenience methods from the URI, I still want to have them in the storage package

Rather than expressing an opinion that you say is blocking can you please provide the reasoning? We have discussed this before and it was regarding separation of concerns. What Charles has recently provided keeps these concerns separate - therefore I would like to understand why you object to this new design. I am, in general, against the idea of providing 2 ways to do all these operations - preferring instead the single, correct, way - that makes documentation, and support, a lot easier.

The list of methods I don’t would introduce in URI is:

In the list is Child and Parent. Unfortunately, as we have identified through some discussions, is that the parent/ child relationship of a URI is not pre-determined by string parsing. I don't see how we can choose to make these optional whilst still having a self-contained URI definition. For LDAP or other non-path-based identifiers the parent or child cannot be inferred. This surely means that the child/parent relationship needs to be returned from the URI - otherwise we start coding specific URI knowledge in other areas, which in my mind would break separation of concerns again.

Mon Jan 4 11:19:52 AM EST 2021 / Charles

This makes the assumption that the consumer of a URI or repository knows more about the external state and therefore need to refresh than the repository does. Unless you can show a real-world use-case where this is true I think that cache management should be handled internally.

OK, granted the repository could add it's own goroutine, mutexes, etc. to deal with this. If the need comes up, we can revisit and possibly add the capability to do this.

Unfortunately not - the URI is a public interface, we cannot just add to it outside of a breaking release. That is basically the whole reason to settle this design ASAP, so that we can settle on a definition of URI that will need to stay for the forseeable future.

Are you opposed to making URI a struct then? I don't see any reason it should continue to be an interface, since the functionality that would accomplish would be handled by the repository system.

Andy 4 Jan:

Are you opposed to making URI a struct then?

Whatever changes seem appropriate, and can be made in a backwards compatible or deprecation-path manner should be explored. I don't know if struct is better than interface without exploring the implementation I guess.

What I will say, however, is that using an interface at the top level allows us to pass around values of that type and yet have the implementation within the storage package. If it becomes a struct then it would follow that this implementation moves out of storage. At this point you can't simply call into storage functions from helper methods on the struct as you would create a circular dependency. The root package should not depend on any child packages - as it contains the definition of all the common types.

Tue Jan 5 01:07:46 PM EST 2021 / Charles

I think this has convinced me that we should follow Tilo's suggestion, and implement these methods in the top-level storage package, rather than as members of the URI interface/struct. I think we want URI to stay an interface for backwards compatibility. In principle, nobody should be implementing their own interface, but it is part of the public API. The philosophical purity of Tilo's argument did not win me over, but the ability to be able to iterate (make additive changes to the API later) is something I think we really want. If it's going to stay as an interface, using package methods also means we have control over the methods (nobody can override URI.Copy if it's storage.Copy instead).

Aside: I have started work on implementing the Repository interface locally, nothing pushed yet. It's taking a while, since I'm also using it as a place to write documentation about what everything is supposed to do.

Tue Jan 5 04:13:09 PM EST 2021 / Charles

I have written a first pass at the Repository interface discussed here.

Tue Jan 5 16:45 PST 2021 / Stuart

Couple of style comments;

  • The Impl suffix is not a convention Fyne has used before. The approach of exporting the interface and hiding the struct typically works better: type FooBar interface {} implemented by type foobar struct {}
  • The pattern of registering a repository with methods to return functions to perform the operations seems overly complex. Couldn't a repository simply register its supported functions? ie storage.RegisterCopier, storage.RegisterNamer, etc. Internally storage would have a map[string]func(params)(returns) for each operation, keyed by URL scheme. ie storage.Copy would use copiers[url.Scheme()] whose value is a type Copier func(URI, URI) error and storage.Name would use namers[url.Scheme()] whose value is a type Namer func(URI) string

Tue Jan 5 08:08:48 PM EST 2021 / Charles

The Impl thing was because I wanted to make it clear that it was returning the implementation of the thing, rather than the thing itself. If this isn't well liked, I am OK with dropping it.

With respect to your other point, I'm not sure it's really any simpler. Also, I would argue that having to explicitly write a function which returns nil makes it harder to accidentally forget to implement something/hook it up. On the other hand, with your approach, the Repository interface would mostly be providing an callback which then calls the appropraite RegisterFoo() methods, which would make it easier to extend in the future, since those methods would no longer be part of the exported Repository interface.

I think if I am being self consistant, I have to agree with your storage.RegisterFoo idea if I am also in support of Tilo's idea of exporting the actual functions themselves in storage rather than as part of the URI interface, which I still am for the same reason (makes it easier to extend without backwards-incompatible changes later).

I may still put the Register*() methods in storage/repository rather than storage, to avoid it being too cluttere, and because most users will not utilzie those methods.

Andy Jan 6:

Thanks, this looks like good progress. I would also worry about adding a repository provider one method at a time - lots of scope to miss certain methods that would result in strange or unusable storage lookups that could easily look like internal errors with Fyne.

It would seem to me that we don't have to take an "all or nothing" approach. We could define the Repository interface that lists the required methods, and then in the future allow the registering of more things outside the core scope. This seems reasonable to me as a partial implementation of the basics just doesn't make sense.

I do agree with Stuart that the "Impl" is not needed - but I also think that returning methods is not needed - we could just implement the functions directly. Look at the work that was done on Theme2 - initially I supported a "partial theme" approach that allowed certain methods to be not present, but I was pushed in the direction of enforcing them all and allowing a hook to use the default functionality. Having gone through that process it feels cleaner. Every implementation is complete and you're not returning nil and then forgetting. Can we match that pattern to simplify this interface and the implementations of it by providing helpful fallback implementaitons where they would be required? With a new storage/repository namespace I imagine that there is less concern about cluttering the main storage API with such utilities? Alternatively, depending on what a default implementation even means, it could be performed by looking up the "file" scheme provider and calling it - perhaps?

Sorry that paragraph was quite long. I will leave it there as there are random ideas scattered in it. But the tl;dr would be that the repository interface functionality looks great, but allowing every method to return nil instead of an implementation could lead to the same partial, and so non-functional, storage providers.

Wed Jan 6 12:47:09 PM EST 2021 / Charles

I will drop the "Impl" thing then. I agree that maybe having the Repository methods return functions is a little clunky.

How do people feel about a hybrid approach, where Repository has functions that directly implement the core functionality required for the repository, along with methods to register or mark as unsupported other functions on a case-by-case basis where those methods are optional.

I think to handle cases where groups of methods have to be implemented, we could have other interfaces. For example, ListableRepository might be an interface that includesIsListable() and List(), and the Register*() function accepts a ListableRepository. This guarantees that both functions are implemented. In other cases where there is just one function to implement, the Register*() method could just accept the function signature as an argument rather than an interface.

To signal something optional isn't supported (e.g. disable the builtin version but don't provide your own), I think the way to go is to still register a function that just returns a NotSupportedError, since that can just be passed along to the helper in storage.

@Andy - I'm not sure what you mean by Theme2? I poked around in theme/ and in some PRs and am still confused. Could you direct link me to it?

Andy 8 Jan:

By Theme2 I just meant the theme.Theme design in the v2.0 implementation. Noticing the DefaultTheme implementation that allows someone to not implement their own icon handling, though it is a required method. They can do:

func Icon(name string) fyne.Resource {
    return DefaultTheme().Icon(name)
}

I don't know if it helps - but as the new approach does not use functions returned from an interface it may be moot.

I'd say that the hybrid you propose is a good solution - we ensure consistency but allow for optional extras. The idea of "future interface for grouping" is a good one - though I hope we won't find too many use cases that were omitted.

My only concern is the usage of UnsupportedError as a flag for using default implementation. I may want to mark an operation as unsupported in my Repository and would be surprised to then find it working using other code. If using errors to propagate the request for a default implementation I think the error should be for that purpose only.

Welcome to the Fyne wiki.

Project documentation

Getting involved

Platform details

Clone this wiki locally