refactor(lib, api, cmd): adjust log methods to use dispatch pattern #1730

ramfox · 2021-03-31T03:50:01Z

The most controversial thing in this pr is I did some cleaning/renaming of log method names and endpoints. The endpoints are less important, since most of them are going away when we refactor to the 0.10.0 qri api standard.

However, the lib method names I can see being a spicy topic.

Why rename? Because we had a few wires crossed and used the same term to mean different things in different cases. This rename ensure that when we talk about Logbook at the LogMethod and api level, we are referring to the entire logbook. When we talk about Log or Entries, we are talking about a list of log entries. We no longer cross wires and refer to a commit history as a Log, as that is reserved for the list of log entries.

lib method name changes

From lib.Log to lib.History - this function does not actually return a list of logs, it returns a list of version infos, aka the history rather than the list of log entries

From lib.Logbook to lib.Entries - this function does not actually return a Logbook, it returns a list of log entries

From lib.PlainLogs to lib.RawLogbook - this function returns an entire logbook as json. The cmd layer referred to this as raw. Raw seemed more intuitive (to me) and like something someone using lib as a library would grok easier. But this one is definitely up for debate. Regardless if we use PlainLogbook or RawLogbook or just Logbook, we need Logbook in there to distinguish this method as returning the entire logbook (rather than a list of log entries).

api changes

The /logbook endpoint (which is on the slate for deprecation), now returns a logbook, rather than a list of log entries.

The /log now returns a list of log entries, which is consistent with the 0.10.0 qri api

- [x] add `Name` & `Attributes` methods on `LogMethods` - [x] add `Log` method to Instance - [x] add `logImpl` - [x] add `Log` `Logbook`, `PlainLogs`, `LogbookSummary` methods to logImpl, that take a scope - [x] dispatch-ify `Log` `Logbook`, `PlainLogs`, and `LogbookSummary` methods - [x] register `Log` with dipatch - [x] replace `NewLogMethods` with `inst.Log` - [x] replace inst in `LogMethod` with `d dispatcher`

Also adjusts `NewHTTPRequestHandler` to use `RespondWithErr` rather than `WriteErrResponse`. `RespondWithErr` examines the returning error and adds the most logical status code to the response. Any tests that previously used `LogHandler`, are now adjusted to create a log handler via the `lib.NewHTTPRequestHandler` & rely on `POST` requests w/ a body.

ramfox · 2021-03-31T03:51:49Z

lib/api.go

+	// AEEntries lists log entries for actions taken on a given dataset
+	AEEntries = APIEndpoint("/log")
+	// AERawLogbook returns the full logbook encoded as human-oriented json
+	AERawLogbook = APIEndpoint("/logbook")
 	// AELogbookSummary returns a string overview of the logbook
 	AELogbookSummary = APIEndpoint("/logbook/summary")
-	// AELogs returns the full logbook encoded as human-oriented json
-	AELogs = APIEndpoint("/logs")


read the main pr comment why I think these are good changes:
#1730 (comment)

ramfox · 2021-03-31T03:52:25Z

lib/log.go

+		"history":        {AEHistory, "POST"},
+		"entries":        {AEEntries, "POST"},
+		"rawlogbook":     {denyRPC, ""},
+		"logbooksummary": {denyRPC, ""},


read the main pr comment for why I think these are good method names:
#1730 (comment)

Arqu

IMHO, don't think this is spicy, this is RIGHT. I had troubles following the full logic on my own on the previous pass.

Left some nitpicks but LGTM.

Arqu · 2021-03-31T07:10:18Z

api/testdata/api.snapshot

@@ -5062,12 +5062,12 @@ HTTP/1.1 200 OK
 Connection: close

 /* snapshot: TestFSIHandlers init case 0: POST / */
-HTTP/1.1 500 Internal Server Error
+HTTP/1.1 400 Bad Request


This case means we don't properly parse/unmarshal the structs or the test is set up wrong. Would be nice to actually fix it as these will be useful for keeping record/testing of how things work until we transition to the new JSON based test framework.

In InitDataset, the first place we check the Ref is dsref.IsValidName, and return dsref.ErrDescribeValidName. This test is supposed to be checking that we error when there is no Ref. The status code change occurred because NewHTTPRequestMethod now uses RespondWithErr rather than WriteErrResponse, with every error returning 500.

We should probably have a dsref.ErrEmtpyRef error and InitDataset should be responding with that instead (and a 422 error probably?).

In RespondWithErr we have dsref.ErrBadCaseShouldRename, dsref.ErrDescribeValidName, and dsref.ErrDescribeValidUsername. Do those make sense as 400 Bad Request errors, or should they be 422 Unprocessable Entity?

Arqu · 2021-03-31T07:11:25Z

lib/log.go

@@ -82,16 +85,93 @@ func (p *LogParams) UnmarshalFromRequest(r *http.Request) error {
 }

 // Log returns the history of changes for a given dataset


Comment needs updating.

Arqu · 2021-03-31T07:12:44Z

lib/log.go

+	// no options yet
+}
+
+// RawLogbook is an alias for a human representation of a plain-old-data logbook


RawLogbook > RawLogs

b5

Agree that these changes are either not spicy, or good spice. Well seasoned PR

We had the concept of `Logs`, as in a list of logs, and `Logbook`, as in the entire log book, switched around in the previous naming. For clarity, I've renamed the endpoints and lib methods: From `lib.Log` to `lib.History` - this function does not actually return a list of logs, it returns a list of version infos, aka the history rather than the list of log entries From `lib.Logbook` to `lib.Entries` - this function does not actually return a Logbook, it returns a list of log entries From `lib.PlainLogs` to `lib.RawLogbook` - this function returns an entire logbook as json. The `cmd` layer referred to this as `Raw`, which feels more like someone who is using lib as a library will grok easier than `Plain`, but this one is definitely up for debate. Regardless if we use `Plain` or `Raw` as a prefix, we should use `Logbook` as the suffix, to distinguish this method as returning the entire logbook (rather than a list of log entries). The `/logbook` endpoint (which is on the slate for deprecation), should probably return a logbook, rather than a list of log entries. `/log` now returns a list of log entries, which is where we are headed in api 0.10.0 anyway

dustmop · 2021-04-02T00:55:57Z

Missed the review on this yesterday, but wanted to double down that all these renames are good! 👍

ramfox added 3 commits March 30, 2021 23:41

refactor(cmd): use inst.Log() to get LogMethods

6368ae7

ramfox commented Mar 31, 2021

View reviewed changes

ramfox requested review from Arqu, b5 and dustmop March 31, 2021 03:52

ramfox self-assigned this Mar 31, 2021

ramfox added the refactor A code change that neither fixes a bug nor adds a feature label Mar 31, 2021

Arqu approved these changes Mar 31, 2021

View reviewed changes

b5 approved these changes Mar 31, 2021

View reviewed changes

ramfox force-pushed the ramfox/refactor_log_dispatch branch from 8dbf160 to d83a1cb Compare March 31, 2021 18:58

ramfox merged commit 75b42e1 into master Mar 31, 2021

ramfox deleted the ramfox/refactor_log_dispatch branch March 31, 2021 19:15

dustmop mentioned this pull request Apr 2, 2021

refactor(api): aligning our API with spec #1734

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(lib, api, cmd): adjust log methods to use dispatch pattern #1730

refactor(lib, api, cmd): adjust log methods to use dispatch pattern #1730

ramfox commented Mar 31, 2021 •

edited

Loading

ramfox Mar 31, 2021

ramfox Mar 31, 2021

Arqu left a comment

Arqu Mar 31, 2021

ramfox Mar 31, 2021

Arqu Mar 31, 2021

Arqu Mar 31, 2021

b5 left a comment

dustmop commented Apr 2, 2021

		@@ -82,16 +85,93 @@ func (p LogParams) UnmarshalFromRequest(r http.Request) error {
		}

		// Log returns the history of changes for a given dataset

refactor(lib, api, cmd): adjust log methods to use dispatch pattern #1730

refactor(lib, api, cmd): adjust log methods to use dispatch pattern #1730

Conversation

ramfox commented Mar 31, 2021 • edited Loading

lib method name changes

api changes

ramfox Mar 31, 2021

Choose a reason for hiding this comment

ramfox Mar 31, 2021

Choose a reason for hiding this comment

Arqu left a comment

Choose a reason for hiding this comment

Arqu Mar 31, 2021

Choose a reason for hiding this comment

ramfox Mar 31, 2021

Choose a reason for hiding this comment

Arqu Mar 31, 2021

Choose a reason for hiding this comment

Arqu Mar 31, 2021

Choose a reason for hiding this comment

b5 left a comment

Choose a reason for hiding this comment

dustmop commented Apr 2, 2021

ramfox commented Mar 31, 2021 •

edited

Loading