refactor: Rename key,id,dockey to docID terminology #1749

shahzadlone · 2023-08-02T15:36:57Z

Relevant issue(s)

Subtask of [Epic] Consistent GQL Renaming #1750 [EPIC]
Resolves Rename instances of _key to _docID #1752
Resolves Make dockey and id request arguments more consistent #1272

BREAKING CHANGE:

Use of _key to access a document's unique id is now deprecated, instead use _docID.
Use of dockey/id is now deprecated, instead use docID.
Use of dockeys/ids is now deprecated, instead use docIDs.

Description

Rename _key to _docID everywhere.
Rename _keys to docIDs in explain the response.
Rename _newKey to _docIDNew in backup/recover functionality.
Fix _docID tests.
Fix explain and backup/recover functionality tests.
Fix the collectionID order for a P2P test (leaving a note as order was reverted).
Update all cids.
Rename all files with key(s)|dockey(s) in the name to doc_id(s).
Document breaking change to pass change detector.

For Reviewers:

Main commits to review are the PR(MAIN) commits.
If you have more time PR(MINOR) and PR(*TEST) commits are good to go over

Disclaimer / Discussion:

I do not like these non-underscored docID/docIDs, would be in favor of underscoring these :

query {
    User(docIDs: ["bae-6a6482a8-24e1-5c73-a237-ca569e41507d"]) {
        _docID
    }
}

query {
    Users {
        Name
        _docID
        _version {
            docID
        }
    }
}

Request: ` {
    commits(groupBy: [docID], order: {docID: DESC}) {
        docID
    }
}`,
Results: []map[string]any{
    {
        "docID": "bae-f54b9689-e06e-5e3a-89b3-f3aee8e64ca7",
    },
    {
        "docID": "bae-72f3dc53-1846-55d5-915c-28c4e83cc891",
    },
},

EDIT: Above was resolved with #2162, to do out of this PR.

Limitations (out of scope of this PR):

examples/schema/booksmany.graphql

api/http/errors.go

fredcarle

Definitely going in the right direction. I added a few comments and then looked at how many other reference to dockey there was. There were hundreds so I create a branch (https://github.com/fredcarle/defradb/tree/fredcarle/chore/dockey-rename) with what I found. There might be more changes needed to cover everything.

fredcarle · 2023-11-24T19:23:11Z

client/request/consts.go

@@ -98,7 +100,7 @@ var (
 	VersionFields = []string{
 		HeightFieldName,
 		CidFieldName,
-		DockeyFieldName,
+		DocID,


todo: I'm pretty sure this needs to be DocIDFieldName

If I apply the above suggestion of To keep things consistent, DocIDFieldName. and then do this then it would mean we change from "docID" to "_docID", which is what I like too. However @jsimnz had a hard opinion to not underscore the field names (yet atleast).

This happens because previously we used dockey/dockeys/id/ids, which now became docID/docIDs.

Note that I am against these (my eyes get cancer from them everytime), also noted in PR description:

query { User(docIDs: ["bae-6a6482a8-24e1-5c73-a237-ca569e41507d"]) { _docID } }

query { Users { Name _docID _version { docID } } }

Request: ` { commits(groupBy: [docID], order: {docID: DESC}) { docID } }`, Results: []map[string]any{ { "docID": "bae-f54b9689-e06e-5e3a-89b3-f3aee8e64ca7", }, { "docID": "bae-72f3dc53-1846-55d5-915c-28c4e83cc891", }, },

My vote is for _ version for all instances of docID(s) (and in future for every internal field :P)

I also have a very strong preference for all internal fields to have _ as it makes it very clear what they are.

client/request/consts.go

merkle/clock/clock_test.go

planner/commit.go

planner/delete.go

shahzadlone · 2023-12-18T09:49:38Z

Definitely going in the right direction. I added a few comments and then looked at how many other reference to dockey there was. There were hundreds so I create a branch (fredcarle/defradb@fredcarle/chore/dockey-rename) with what I found. There might be more changes needed to cover everything.

Yes most of those are local vars, description strings, comments and test function names. Can take care of them too within this PR if there is preference

shahzadlone · 2024-01-03T17:31:16Z

tests/integration/schema/get_schema_test.go

question: @AndrewSisley is the order change here expected?

Not off the top of my head, I can't remember what order the results we returned in - there is a chance it is sorted by version id though

AndrewSisley · 2024-01-03T17:40:49Z

thought: I am finding this really hard to effectively review. There are a lot of independent changes here, and reviewing them all at once with them blended into each other is quite tricky. This is made harder by the nature of the changes (renaming).

The commits do not help reviewers as much as they could, as they are not what I would consider complete - where a commit begins a change (e.g. Rename _key to _dockey), but leaves the code in a broken state to be patched by later commits (e.g. Apply naming within unit tests). This means as a reviewer I cannot effectively review the rename of _key to _dockey unless I review a guessed range of commits (and hope nothing else is interleaved within that range).

In the future I would suggest either breaking up the PR (I understand why you may want it in one here). Or broadening the scope of the commits so that reviewers can review a single, complete, build-passes change in a single commit.

I'm not yet asking for you to change this in this PR, but thought I'd leave the feedback.

shahzadlone · 2024-01-03T18:00:26Z

thought: I am finding this really hard to effectively review. There are a lot of independent changes here, and reviewing them all at once with them blended into each other is quite tricky. This is made harder by the nature of the changes (renaming).

The commits do not help reviewers as much as they could, as they are not what I would consider complete - where a commit begins a change (e.g. Rename _key to _dockey), but leaves the code in a broken state to be patched by later commits (e.g. Apply naming within unit tests). This means as a reviewer I cannot effectively review the rename of _key to _dockey unless I review a guessed range of commits (and hope nothing else is interleaved within that range).

In the future I would suggest either breaking up the PR (I understand why you may want it in one here). Or broadening the scope of the commits so that reviewers can review a single, complete, build-passes change in a single commit.

I'm not yet asking for you to change this in this PR, but thought I'd leave the feedback.

I appreciate the feedback and understand the concern, however due to the nature of this PR (renaming). I had to pick between either (1) smaller / consumable / somewhat structured but not clean commits (as the build might fail), OR (2) according to your suggestion range of all commits that form into a clean state as one big commit (i.e. passing build / complete change).

I went with (1) as according to the nature of the PR the review is mainly about weather the renaming makes sense or not, with the assumption that no logic has changed hence it would build. (2) would also in some cases require to change cids multiple times, for individual complete change to pass the build, or like mentioned a really large commit body if I want to change cids once (which to be fair is possible and very easy for me to create even now, I can merge quickly all renaming change commits into one).

Nonetheless will keep this in mind in future for non-rename PRs, I do normally keep commits clean as much as possible.

AndrewSisley · 2024-01-03T18:49:20Z

would also in some cases require to change cids multiple times, for individual complete change to pass the build

This is fine, and would quite probably be a net time time saver for the team in a 3000 line PR. You could alternatively sacrifice the build and bundle the cid changes (only) into a single commit at the end.

cli/cli.go

cli/version_test.go

AndrewSisley

Some small comments submitted now as I'm context switching mid-review.

client/collection.go

client/doc_id.go

client/document.go

AndrewSisley · 2024-01-04T14:43:24Z

client/errors.go

@@ -32,23 +32,16 @@ const (
 // This list is incomplete and undefined errors may also be returned.
 // Errors returned from this package may be tested against these errors with errors.Is.
 var (


question: Why were so many errors deleted? Were they unused?

Yes, all deleted errors were unused. It's because the linter doesn't catch unused public variables when they are become unused by developers, the same way it catches unused private variables.

Okay, thanks then for deleting them :)

AndrewSisley · 2024-01-04T14:44:35Z

client/request/consts.go

@@ -36,14 +32,21 @@ const (
 	OrderClause   = "order"
 	DepthClause   = "depth"

+	DocIDArgName  = "docID"


praise: These are much better const names than the originals, thank you :)

AndrewSisley · 2024-01-04T14:50:10Z

client/request/consts.go

 	GroupFieldName   = "_group"
 	DeletedFieldName = "_deleted"
 	SumFieldName     = "_sum"
 	VersionFieldName = "_version"

+	// New generated document id from a backed up document,
+	// which might have a different _docID originally.
+	NewDocIDFieldName = "_docIDNew"


question: Why has the position of new in this constant changed?

Previously when we had a _key, we had a corresponding _newKey within backup and recover functionality to mark existing and new keys.

Now with the introduction of a camel case name: _docID, I didn't like _newdocID for obvious reasons nor _newDocID for the simple reason that if we want to do a regex search in the future for all usages of docID it won't match.

So I went with _docIDNew.

😅 I'm not sure a slightly simpler regex search is worth the relatively unexpected 'new' location. But I'm not going to push for a change here, it isn't very important to me.

keeping as is, unless someone has a stronger opinion

It's just used in backups so it's low impact. I'm ok with _docIDNew.

db/base/collection_keys.go

db/collection.go

db/base/collection_keys.go

AndrewSisley

Looks good to me, just some small comments added. Have now reviewed the full PR.

praise: Thank you very much for spreading further the use of the request consts, and cleaning up their names.

Strongly suggest getting in touch with Fred when you are looking to merge this, he has an open PR atm that overlaps a fair amount with this one.

AndrewSisley · 2024-01-04T16:31:22Z

docs/cli/defradb_client_document.md

@@ -1,38 +0,0 @@
-## defradb client document
-
-Create, read, update, and delete documents.


question: Why have these files been deleted?

These seem outdated docs, do lemme know if anything isn't outdated according to you but I deleted (my guess is that overtime the make docs command generates new documentation, but does not remove old ones).

These are auto-generated?

Yea so for example docs/cli/defradb_client_collection_docIDs.md was regenerated with new file name but was originally docs/cli/defradb_client_collection_keys.md

And this file that I deleted: defradb_client_document.md I am pretty sure has been out dated by defradb_client_collection.md: https://github.com/shahzadlone/defradb/blob/gql-renaming-phase1/docs/cli/defradb_client_collection.md

Ah okay nice - I did not know this :)

I've opened #2166 and #2165

I didn't either LOL,

Cheers for opening these

planner/commit.go

planner/type_join.go

shahzadlone · 2024-01-04T17:19:08Z

Looks good to me, just some small comments added. Have now reviewed the full PR.

praise: Thank you very much for spreading further the use of the request consts, and cleaning up their names.

Strongly suggest getting in touch with Fred when you are looking to merge this, he has an open PR atm that overlaps a fair amount with this one.

Cheers thanks for reviewing this PR :), for sure will coordinate with @fredcarle

fredcarle · 2024-01-04T19:36:03Z

db/fetcher/versioned.go

@@ -429,7 +429,7 @@ func (vf *VersionedFetcher) Close() error {
 }

 // NewVersionedSpan creates a new VersionedSpan from a DataStoreKey and a version CID.
-func NewVersionedSpan(dockey core.DataStoreKey, version cid.Cid) core.Spans {
+func NewVersionedSpan(dsKey core.DataStoreKey, version cid.Cid) core.Spans {


praise: This was a confusing one before this change. Thanks for fixing!

db/collection_delete.go

db/collection_get.go

fredcarle

LGTM! Thanks a lot for doing this Shahzad. It a really nice change and I love that it removes the confusion I sometimes had when not paying too too much attention between dockey and datastore key. Now it's unmistakable.

Please resolve the conversations before merging :)

Done

## Relevant issue(s) - Subtask of sourcenetwork#1750 [EPIC] - [x] Resolves sourcenetwork#1752 - [x] Resolves sourcenetwork#1272 ## BREAKING CHANGE: - Use of `_key` to access a document's unique id is now deprecated, instead use `_docID`. - Use of `dockey`/`id` is now deprecated, instead use `docID`. - Use of `dockeys`/`ids` is now deprecated, instead use `docIDs`. ## Description - [x] Rename `_key` to `_docID` everywhere. - [x] Rename `_keys` to `docIDs` in explain the response. - [x] Rename `_newKey` to `_docIDNew` in backup/recover functionality. - [x] Fix `_docID` tests. - [x] Fix explain and backup/recover functionality tests. - [x] Fix the collectionID order for a P2P test (leaving a note as order was reverted). - [x] Update all cids. - [x] Rename all files with `key(s)|dockey(s)` in the name to `doc_id(s)`. - [x] Document breaking change to pass change detector. ## For Reviewers: - Main commits to review are the `PR(MAIN)` commits. - If you have more time `PR(MINOR) and PR(*TEST)` commits are good to go over ## Disclaimer / Discussion: I do not like these non-underscored `docID/docIDs`, would be in favor of underscoring these : ``` query { User(docIDs: ["bae-6a6482a8-24e1-5c73-a237-ca569e41507d"]) { _docID } } ``` ``` query { Users { Name _docID _version { docID } } } ``` ``` Request: ` { commits(groupBy: [docID], order: {docID: DESC}) { docID } }`, Results: []map[string]any{ { "docID": "bae-f54b9689-e06e-5e3a-89b3-f3aee8e64ca7", }, { "docID": "bae-72f3dc53-1846-55d5-915c-28c4e83cc891", }, }, ``` EDIT: Above was resolved with sourcenetwork#2162, to do out of this PR. ## Limitations (out of scope of this PR): - sourcenetwork#1467 - sourcenetwork#1751 - sourcenetwork#1550 - sourcenetwork#2162

shahzadlone added area/testing Related to any test or testing suite code quality Related to improving code quality deprecate Indicates something is deprecated. labels Aug 2, 2023

shahzadlone self-assigned this Aug 2, 2023

shahzadlone added this to the DefraDB v0.7 milestone Aug 2, 2023

shahzadlone commented Aug 2, 2023

View reviewed changes

examples/schema/booksmany.graphql Outdated Show resolved Hide resolved

shahzadlone commented Aug 2, 2023

View reviewed changes

api/http/errors.go Outdated Show resolved Hide resolved

shahzadlone force-pushed the gql-renaming-phase1 branch from 5e19d45 to c3b1964 Compare August 3, 2023 11:27

shahzadlone changed the title ~~refac: Rename _key to _docID as unique id~~ refac: Rename 'key' terminology to 'docID' Aug 3, 2023

shahzadlone changed the title ~~refac: Rename 'key' terminology to 'docID'~~ refac: Rename '_key' terminology to '_docID' Aug 3, 2023

shahzadlone changed the title ~~refac: Rename '_key' terminology to '_docID'~~ refac: Rename key terminology to docID Aug 3, 2023

shahzadlone changed the title ~~refac: Rename key terminology to docID~~ refac: Rename _key terminology to _docID Aug 3, 2023

shahzadlone changed the title ~~refac: Rename _key terminology to _docID~~ refactor: Rename _key to _docID terminology Aug 3, 2023

shahzadlone mentioned this pull request Sep 6, 2023

test(i): Restructure mutation integration test dir #1848

Merged

shahzadlone force-pushed the gql-renaming-phase1 branch 2 times, most recently from 1283538 to f102b7d Compare September 7, 2023 05:36

shahzadlone force-pushed the gql-renaming-phase1 branch 2 times, most recently from f102b7d to 33d5f6a Compare September 14, 2023 12:57

shahzadlone force-pushed the gql-renaming-phase1 branch 4 times, most recently from b761bde to 6305cb5 Compare October 5, 2023 03:56

shahzadlone force-pushed the gql-renaming-phase1 branch from 6305cb5 to b0fdfcc Compare October 5, 2023 19:45

shahzadlone force-pushed the gql-renaming-phase1 branch from b0fdfcc to e21a373 Compare October 19, 2023 19:14

fredcarle reviewed Nov 24, 2023

View reviewed changes

shahzadlone force-pushed the gql-renaming-phase1 branch 4 times, most recently from f4ba7a8 to 853cb77 Compare December 21, 2023 06:45

shahzadlone requested a review from fredcarle January 3, 2024 17:05

shahzadlone mentioned this pull request Jan 3, 2024

Consistent underscoring, prepend non-argument types with (_) #2162

Open

shahzadlone changed the title ~~refactor: Rename _key to _docID terminology~~ refactor: Rename key(s),id(s),dockey(s) to docID(s) terminology Jan 3, 2024

shahzadlone changed the title ~~refactor: Rename key(s),id(s),dockey(s) to docID(s) terminology~~ refactor: Rename key,id,dockey to docID terminology Jan 3, 2024

shahzadlone commented Jan 3, 2024

View reviewed changes

AndrewSisley reviewed Jan 4, 2024

View reviewed changes

cli/cli.go Show resolved Hide resolved

AndrewSisley reviewed Jan 4, 2024

View reviewed changes

cli/version_test.go Show resolved Hide resolved

AndrewSisley requested changes Jan 4, 2024

View reviewed changes

AndrewSisley previously requested changes Jan 4, 2024

View reviewed changes

fredcarle reviewed Jan 4, 2024

View reviewed changes

db/collection_delete.go Outdated Show resolved Hide resolved

fredcarle reviewed Jan 4, 2024

View reviewed changes

db/collection_delete.go Outdated Show resolved Hide resolved

fredcarle reviewed Jan 4, 2024

View reviewed changes

db/collection_get.go Outdated Show resolved Hide resolved

fredcarle reviewed Jan 4, 2024

View reviewed changes

db/collection_get.go Outdated Show resolved Hide resolved

fredcarle approved these changes Jan 4, 2024

View reviewed changes

shahzadlone modified the milestones: DefraDB v0.7, DefraDB v0.9 Jan 4, 2024

PR(ADHERE-FEEDBACK): Make review suggestions.

958d215

shahzadlone force-pushed the gql-renaming-phase1 branch from 89f408f to 958d215 Compare January 4, 2024 22:31

shahzadlone requested a review from AndrewSisley January 4, 2024 22:31

shahzadlone merged commit 9da4dab into sourcenetwork:develop Jan 4, 2024
58 of 59 checks passed

shahzadlone deleted the gql-renaming-phase1 branch January 4, 2024 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Rename key,id,dockey to docID terminology #1749

refactor: Rename key,id,dockey to docID terminology #1749

shahzadlone commented Aug 2, 2023 •

edited

Loading

fredcarle left a comment

fredcarle Nov 24, 2023

shahzadlone Dec 18, 2023 •

edited

Loading

fredcarle Dec 18, 2023

shahzadlone commented Dec 18, 2023 •

edited

Loading

shahzadlone Jan 3, 2024

AndrewSisley Jan 3, 2024 •

edited

Loading

AndrewSisley commented Jan 3, 2024 •

edited

Loading

shahzadlone commented Jan 3, 2024 •

edited

Loading

AndrewSisley commented Jan 3, 2024 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Jan 4, 2024

shahzadlone Jan 4, 2024 •

edited

Loading

AndrewSisley Jan 4, 2024

AndrewSisley Jan 4, 2024

AndrewSisley Jan 4, 2024

shahzadlone Jan 4, 2024

AndrewSisley Jan 4, 2024

shahzadlone Jan 4, 2024

fredcarle Jan 4, 2024

AndrewSisley left a comment

AndrewSisley Jan 4, 2024

shahzadlone Jan 4, 2024 •

edited

Loading

AndrewSisley Jan 4, 2024

shahzadlone Jan 4, 2024 •

edited

Loading

AndrewSisley Jan 4, 2024

shahzadlone Jan 4, 2024

shahzadlone commented Jan 4, 2024

fredcarle Jan 4, 2024

fredcarle left a comment

		@@ -1,38 +0,0 @@
		## defradb client document

		Create, read, update, and delete documents.

refactor: Rename key,id,dockey to docID terminology #1749

refactor: Rename key,id,dockey to docID terminology #1749

Conversation

shahzadlone commented Aug 2, 2023 • edited Loading

Relevant issue(s)

BREAKING CHANGE:

Description

For Reviewers:

Disclaimer / Discussion:

Limitations (out of scope of this PR):

fredcarle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone commented Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Jan 3, 2024 • edited Loading

Choose a reason for hiding this comment

AndrewSisley commented Jan 3, 2024 • edited Loading

shahzadlone commented Jan 3, 2024 • edited Loading

AndrewSisley commented Jan 3, 2024 • edited Loading

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahzadlone commented Jan 4, 2024

Choose a reason for hiding this comment

fredcarle left a comment

Choose a reason for hiding this comment

shahzadlone commented Aug 2, 2023 •

edited

Loading

shahzadlone Dec 18, 2023 •

edited

Loading

shahzadlone commented Dec 18, 2023 •

edited

Loading

AndrewSisley Jan 3, 2024 •

edited

Loading

AndrewSisley commented Jan 3, 2024 •

edited

Loading

shahzadlone commented Jan 3, 2024 •

edited

Loading

AndrewSisley commented Jan 3, 2024 •

edited

Loading

shahzadlone Jan 4, 2024 •

edited

Loading

shahzadlone Jan 4, 2024 •

edited

Loading

shahzadlone Jan 4, 2024 •

edited

Loading