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

lib/ should be refactored so it's just a thin layer over actions #519

Closed
dustmop opened this issue Aug 20, 2018 · 1 comment · Fixed by #543
Closed

lib/ should be refactored so it's just a thin layer over actions #519

dustmop opened this issue Aug 20, 2018 · 1 comment · Fixed by #543
Assignees

Comments

@dustmop
Copy link
Contributor

dustmop commented Aug 20, 2018

The lib/ directory, responsible for being the entry point that both cmd/ and api/ call, should be little more than a thin layer over the functions in (repo/)actions/, dispatching calls over rpc or p2p.Node based upon whether qri connect is running or whether the datasetref parameter is locally available or not.

The fact that this doesn't happen currently is the source of a lot of inconsistent behavior, bugs, and missing functionality. For example, many commands fail to work when qri connect is running, even if they need to simply access local data. Other commands fail to work on remote dataset refs even when they should. Changing each command one-by-one to fix these bugs is slow-going and error-prone; better to fix the architecture instead.

Currently, commands fall into three camps:

  1. Working as expected
  • Select: calls actions.Select for local datasetrefs, and Node.RequestDataset for remote datasetrefs (this is used by qri get)
  • List: calls repo.References for yourself, and Node.RequestDatasetList for peers
  • Add: N/A for local datasetrefs, and calls Node.RequestDataset for remote datasetrefs
    (that's it)
  1. Broken for remote nodes, even though it should work
  • Get: calls dsfs.LoadDataset for local datasetrefs, fails for remote datasetrefs (this is used by api's GET, should be combined with Select)
  • Info: uses Get, inherits the same problems
  • Render: calls dsfs.LoadDataset for local datasetrefs, displays the error "could not find dataset" for remote datasets
  1. Broken when running qri connect, regardless of whether the dataset is local or not

All the following commands exhibit this behavior, caused by the fact that they call cmd/qri.go's function QriOptions.Repo(), which throws an error if qri connect is running:

  • Body
  • Log
  • Export

This is not a hard requirement though, these functions should instead dispatching the call through the rpc client.

Additionally, in cases where the lib/ code is calling dsfs/ functionality (List, Get, Render) instead of functions in actions/, a new action should be created to remove this direct dependency. See #183.

@dustmop
Copy link
Contributor Author

dustmop commented Aug 21, 2018

Here's a handy table I made while investigating the current state of the world:

       +---------------------------------------------------------------------------------+
       |CanonicalizeDatasetRef                 |                                         |
       |local                                  |remote (repo.ErrNotFound)                |
       +---------------------------------------+-----------------------------------------+
       |offline           |qri connect         |offline          |qri connect            |notes
       +------------------+--------------------+-----------------+-----------------------+-------------
Select |actions.Select    |actions.Select      |"not found"      |Node.RequestDataset    |`qri get ...`
List   |repo.References   |repo.References     |"cannot list"    |Node.RequestDatasetList|`qri list`
Get    |dsfs.LoadDataset  |N/A (api only)      |"not found"      *(broken)               | api /GET/
Info   |dsr.Get           |dsr.Get             |"not found"      *(broken) - displays "0"|
Render |dsfs.LoadDataset  |dsfs.LoadDataset    |"not found"      *"repo not found"       |
Body   |dsfs.LoadDataset  *"repo not avail"    |"not found"      *"repo not avail"       |
Log    |repo.ReadDataset  *"repo not avail"    |"not found"      *"repo not avail"       |
Export |dsr.Get           *"repo not avail"    |"not found"      *"repo not avail"       | 
New    |repo.CreateDataset|repo.CreateDataset  |N/A              |N/A
Save   |repo.CreateDataset|repo.CreateDataset  |N/A              |N/A
Remove |repo.DeleteDataset|repo.DeleteDataset  |N/A              |N/A

Ideally, all of these columns should be full of "actions.{SomeMethod}", except the third column, which represents trying to access a remote dataset while not connected and necessarily should be an error, and the fourth column which should be using "Node.{SomeMethod}" to send requests to connected peers. Any cell beginning with an asterisk ("*") is currently broken behavior.

dustmop added a commit that referenced this issue Aug 22, 2018
Get method written so that it is used by both cmd/ and api/. Follows the new
guideline set out by #519, moving nearly all the logic from lib/ to actions/.
Logic which is specific to the command-line, selecting parts of a dataset
using a path, moved up a level to the cmd/ package.

Get can now be used to retrieve info about local and remote datasets, from
both command-line and api server. Fixes #509, #479 #397, and possibly others.

Clears the way to merging `body` into `get` in a future PR.

Breaks ability to `get` multiple datasets at once.
dustmop added a commit that referenced this issue Aug 22, 2018
Get method written so that it is used by both cmd/ and api/. Follows the new
guideline set out by #519, moving nearly all the logic from lib/ to actions/.
Logic which is specific to the command-line, selecting parts of a dataset
using a path, moved up a level to the cmd/ package.

Get can now be used to retrieve info about local and remote datasets, from
both command-line and api server. Fixes #509, #479 #397, and possibly others.

Clears the way to merging `body` into `get` in a future PR.

Breaks ability to `get` multiple datasets at once.
dustmop added a commit that referenced this issue Aug 23, 2018
Get method written so that it is used by both cmd/ and api/. Follows the new
guideline set out by #519, moving nearly all the logic from lib/ to actions/.
Logic which is specific to the command-line, selecting parts of a dataset
using a path, moved up a level to the cmd/ package.

Get can now be used to retrieve info about local and remote datasets, from
both command-line and api server. Fixes #509, #479 #397, and possibly others.

Clears the way to merging `body` into `get` in a future PR.

Breaks ability to `get` multiple datasets at once.
@ghost ghost assigned b5 Sep 11, 2018
@ghost ghost added the in progress label Sep 11, 2018
@b5 b5 closed this as completed in #543 Sep 13, 2018
@ghost ghost removed the in progress label Sep 13, 2018
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 a pull request may close this issue.

2 participants