Document labels: use fields other than _id wherever possible #569

PrashanthCorp · 2018-04-28T00:33:03Z

Fixes #381
Fixes #145

PrashanthCorp · 2018-04-28T00:39:53Z

src/docdb/tree/DocDBDocumentTreeItem.ts

@@ -33,7 +34,9 @@ export class DocDBDocumentTreeItem implements IAzureTreeItem {
    }

    public get label(): string {
-        return this.document.id;
+        const presentFields = documentDefaultFields.filter(element => this._document.hasOwnProperty(element));
+        const canonicalField = presentFields.find((element) => this._document[element] && this._document[element].toString() && !this._document[element].toString().startsWith("[object"));


The three conditions in the find :

Removes values being false-y (null, undefined)

Removes cases where the toString is blank ""

Removes cases where this._document[element] is an Object which does not have it's own implementation. In such cases, the toString value returned is one of [object Object], [object null], [object undefined] etc.

A simple type check does not work because typeof <ObjectID> is object ¯\_(ツ)_/¯

StephenWeatherford · 2018-04-28T00:51:40Z

src/constants.ts

@@ -32,3 +32,5 @@ var isAccepted = collection.queryDocuments(

 if (!isAccepted) throw new Error('The query was not accepted by the server.');
 };`
+
+export const documentDefaultFields = ["name", "Name", "ID", "UUID", "id", "_id"];


Are the DBs case-sensitive?

StephenWeatherford · 2018-04-28T00:55:03Z

src/docdb/tree/DocDBDocumentTreeItem.ts

@@ -33,7 +34,9 @@ export class DocDBDocumentTreeItem implements IAzureTreeItem {
    }

    public get label(): string {
-        return this.document.id;
+        const presentFields = documentDefaultFields.filter(element => this._document.hasOwnProperty(element));
+        const canonicalField = presentFields.find((element) => this._document[element] && this._document[element].toString() && !this._document[element].toString().startsWith("[object"));


typeof == "string"

StephenWeatherford · 2018-04-28T00:57:57Z

src/docdb/tree/DocDBDocumentTreeItem.ts

@@ -33,7 +34,9 @@ export class DocDBDocumentTreeItem implements IAzureTreeItem {
    }

    public get label(): string {
-        return this.document.id;
+        const presentFields = documentDefaultFields.filter(element => this._document.hasOwnProperty(element));
+        const canonicalField = presentFields.find((element) => this._document[element] && this._document[element].toString() && !this._document[element].toString().startsWith("[object"));


multiple calls to toString()

StephenWeatherford · 2018-04-28T03:44:59Z

src/docdb/tree/DocDBDocumentTreeItem.ts

@@ -18,24 +19,22 @@ export class DocDBDocumentTreeItem implements IAzureTreeItem {
    public readonly commandId: string = 'cosmosDB.openDocument';

    public readonly partitionKeyValue: string | undefined;
+    public label: string;


private _label:string;

public get label() {
return this._label;
etc.

ditto for next class

The update operation for the collection view in Mongo would have the side-effect of updating a document's label too. In order for that to work, the label field needs a setter. After the offline discussion, it looks like a private+getter+setter is preferred as opposed to public.

StephenWeatherford · 2018-04-28T03:46:11Z

src/constants.ts

@@ -32,3 +32,5 @@ var isAccepted = collection.queryDocuments(

 if (!isAccepted) throw new Error('The query was not accepted by the server.');
 };`
+
+export const documentDefaultFields = ["name", "Name", "ID", "UUID", "id", "_id"];


documentLabelFields?

StephenWeatherford · 2018-04-28T03:48:00Z

src/docdb/tree/DocDBDocumentTreeItem.ts

@@ -87,6 +86,7 @@ export class DocDBDocumentTreeItem implements IAzureTreeItem {
                        }
                    });
            });
+            this.label = getDocumentTreeItemLabel(this._document);


don't you have to call refreshLabel or something?

StephenWeatherford · 2018-04-28T03:50:17Z

src/utils/vscodeUtils.ts

+export function getDocumentTreeItemLabel(document: IMongoDocument | RetrievedDocument): string {
+    for (let field of documentDefaultFields) {
+        if (document.hasOwnProperty(field)) {
+            if (document[field]) { //ignore if false-y value: null, undefined, "".


let value = document[field];
if (value) ...

Slightly better perf, but more importantly, less chance of mistakes or botched bug fixes.

StephenWeatherford · 2018-04-28T03:53:27Z

src/utils/vscodeUtils.ts

+                } else { // A simple instanceOf check doesn't seem to work. I run into this issue detailed here :
+                    //https://libertyseeds.ca/2015/01/03/Mongo-ObjectID-fails-instanceof-type-test-in-nodeunit/
+                    const result = document[field].toString();
+                    if (!result.startsWith("[object ")) {


Still not 100% convinced, but I guess we'll see if we hit any issues.

StephenWeatherford · 2018-05-19T01:14:48Z

What's the status of this?

StephenWeatherford · 2018-05-21T19:54:46Z

src/docdb/editors/DocDBDocumentNodeEditor.ts

@@ -24,7 +24,9 @@ export class DocDBDocumentNodeEditor implements ICosmosEditor<RetrievedDocument>
    }

    public async update(document: RetrievedDocument): Promise<RetrievedDocument> {
-        return await this._documentNode.treeItem.update(document);
+        const updatedDoc = await this._documentNode.treeItem.update(document);
+        this._documentNode.refresh();


Do you need to wait for the promise?

I'd say you should unless there's a reason you specifically think we shouldn't.

StephenWeatherford · 2018-05-21T20:11:54Z

src/docdb/tree/DocDBDocumentTreeItem.ts

@@ -52,7 +55,7 @@ export class DocDBDocumentTreeItem implements IAzureTreeItem {
    }

    public async deleteTreeItem(_node: IAzureNode): Promise<void> {
-        const message: string = `Are you sure you want to delete document '${this.label}'?`;
+        const message: string = `Are you sure you want to delete document '${this._label}'?`;


nit: Since you have a reader for "label" you should probably still use "label" here.

StephenWeatherford · 2018-05-21T20:13:31Z

src/mongo/editors/MongoCollectionNodeEditor.ts

@@ -38,6 +38,8 @@ export class MongoCollectionNodeEditor implements ICosmosEditor<IMongoDocument[]
            const documentNode = documentNodes.find((node) => node.treeItem.document._id.toString() === updatedDoc._id.toString());
            if (documentNode) {
                documentNode.treeItem.document = updatedDoc;
+                documentNode.treeItem.label = getDocumentTreeItemLabel(documentNode.treeItem.document);


I think the right thing to do here is:

put the logic to set _label in refreshLabel()

call documentNode.refresh() whenever you want the label to get refreshed

StephenWeatherford · 2018-05-30T17:13:27Z

src/docdb/tree/DocDBDocumentTreeItem.ts

    }

    public get id(): string {
        return this.document.id;
    }

-    public get label(): string {
-        return this.document.id;
+    public async refreshLabel() {


return value
no need to be async

Good observation.

However, IAzureTreeItem asks for a refreshLabel() that returns a Promise<void> Not having it async or a return type of void instead of Promise<void> gives type errors.

This is also the case in the MongoDocTreeItem class.

I'll add the return type on the function header to make it clearer. :)

StephenWeatherford · 2018-05-30T17:18:11Z

src/utils/vscodeUtils.ts

+    for (let field of documentLabelFields) {
+        if (document.hasOwnProperty(field)) {
+            let value = document[field];
+            if (value) { //ignore if false-y value: null, undefined, "".


This will also ignore zero, which I don't think would be desirable.

Good catch.

Looking at the other falsy values, I think we'll be covered after acconting for 0.

In this case I'd suggest the following:

I'd suggest this:
(note: you're effectively checking that it's not an object with your value.toString().startsWith("[object ")) below. I'm not sure this idea behavior when the property to display is an object or array, but it seems reasonable to skip it and try something else. But typeof is a lot better than the toString.startsWith()

// If doesn't have value, or is null, ObjectId or other object type, try a different property
if (typeof value !== 'undefined' && typeof value !== 'object') {
return String(value);
}

I think that handles it all, right?

Yeah. It relies on null having type object (slightly obfuscated). If that's well known, then I'll go ahead with it.

Unfortunately, typeof null = "object" in JS. Well-known "bug" that can't be fixed now.

StephenWeatherford · 2018-05-30T17:19:20Z

src/utils/vscodeUtils.ts

+            }
+        }
+    }
+    return document["_id"].toString();


Can _id be undefined or null?

Yes. In case of SQL documents, the id field should be present.

I have both these fields in the documentLabelFields, so they should be covered in the loop. What would a good default return be to ensure type-safety?

I'd leave it as _id, since we'll want to change documentLabelFields to be user-settable anyway. Looks like previous code had return document._id.toString(). Still, I'd rather be conservative and not assume this won't give a null ref exception.

I think that String(document._id) is safe for null/undefined? Can you verify? Thx.

StephenWeatherford · 2018-05-30T17:20:37Z

Have you reviewed this yourself and looked for issues I might find? There's at least one more issue I haven't mentioned. Thanks.

PrashanthCorp · 2018-05-31T22:59:14Z

src/mongo/editors/MongoDocumentNodeEditor.ts

@@ -25,7 +25,9 @@ export class MongoDocumentNodeEditor implements ICosmosEditor<IMongoDocument> {
    }

    public async update(document: IMongoDocument): Promise<IMongoDocument> {
-        return await this._documentNode.treeItem.update(document);
+        const updatedDoc = await this._documentNode.treeItem.update(document);
+        this._documentNode.refresh();


Forgot to await this.

StephenWeatherford · 2018-05-31T23:32:01Z

src/mongo/editors/MongoFindOneResultEditor.ts

        if (node) {
-            return (<IAzureNode<MongoDocumentTreeItem>>node).treeItem.update(newDocument);
+            result = await (<IAzureNode<MongoDocumentTreeItem>>node).treeItem.update(newDocument);


This is the other issue I saw. Why was the return removed here?

My bad. We need to kick off the refresh before returning, but after updating the value.

PrashanthCorp · 2018-06-04T18:53:36Z

src/mongo/editors/MongoFindOneResultEditor.ts

+        } else {
+            // If the node isn't cached already, just update it to Mongo directly (without worrying about updating the tree)
+            const db = await this._databaseNode.treeItem.getDb();
+            result = await MongoDocumentTreeItem.update(db.collection(this._collectionName), newDocument);


Felt this control flow looked clearer. If you prefer a return statement after each block (functionally the same), I'll do that.

I'm fine with either one, really depends on the rest of the code as to which is easier to read/maintain (e.g. how many control flows, how big the then case is, etc.)

StephenWeatherford · 2018-06-06T18:28:55Z

src/constants.ts

@@ -34,3 +34,5 @@ export const defaultStoredProcedure =
 };` ;

 export const emptyPartitionKeyValue = {};
+
+export const documentLabelFields = ["name", "Name", "ID", "UUID", "id", "_id"];


Please add "Id", "uuid"

StephenWeatherford · 2018-06-06T18:33:39Z

src/utils/vscodeUtils.ts

+    for (let field of documentLabelFields) {
+        if (document.hasOwnProperty(field)) {
+            let value = document[field];
+            if (value || value === 0) { //ignore if false-y value: null, undefined, "".


You were supposed to remove this line:

if (value || value === 0)

StephenWeatherford · 2018-06-06T18:33:56Z

src/utils/vscodeUtils.ts

+        if (document.hasOwnProperty(field)) {
+            let value = document[field];
+            if (value || value === 0) { //ignore if false-y value: null, undefined, "".
+                if (value != 'undefined' && typeof value !== 'object') {


typeof value

StephenWeatherford · 2018-06-12T20:08:44Z

@PrashanthCorp CLA will pass if you push a new commit

PrashanthCorp · 2018-06-19T17:34:42Z

Ping.

PrashanthCorp requested a review from StephenWeatherford April 28, 2018 00:33

PrashanthCorp commented Apr 28, 2018

View reviewed changes

StephenWeatherford reviewed Apr 28, 2018

View reviewed changes

Prashanth added 2 commits May 15, 2018 11:58

Update third party notices for mongodb-extended-json.

c4a2ebf

Merge branch 'master' of https://github.com/Microsoft/vscode-cosmosdb

cdb3f9f

StephenWeatherford reviewed May 21, 2018

View reviewed changes

Prashanth added 7 commits May 21, 2018 14:32

Merge branch 'master' of https://github.com/Microsoft/vscode-cosmosdb

ddf820d

Document labels: use fields other than _id wherever possible

997a278

Remove the number of calls to toString in finding the label

5a06dcb

Force refresh a document's tree item post-update. Fixes #145

ae7e176

Document labels : make private

1c84a64

Add setter for tsc -p

761e597

Changes based on feedback

f095bfb

PrashanthCorp force-pushed the ps/381 branch from 3e0161b to f095bfb Compare May 21, 2018 21:53

StephenWeatherford reviewed May 30, 2018

View reviewed changes

PrashanthCorp commented May 31, 2018

View reviewed changes

StephenWeatherford reviewed May 31, 2018

View reviewed changes

Prashanth added 2 commits May 31, 2018 18:51

Based on feedback

ba4a7cd

tsc fix

a1ba49e

PrashanthCorp commented Jun 4, 2018

View reviewed changes

StephenWeatherford reviewed Jun 6, 2018

View reviewed changes

Feedback + tests

ae8a427

Add field to documentLabelFields. Kicks off CLA.

0cc96aa

StephenWeatherford approved these changes Jun 19, 2018

View reviewed changes

PrashanthCorp merged commit e6fa015 into master Jun 19, 2018

StephenWeatherford mentioned this pull request Jun 26, 2018

Add setting for document field labels #719

Merged

PrashanthCorp deleted the ps/381 branch June 29, 2018 17:27

PrashanthCorp mentioned this pull request Jul 6, 2018

An invalid document should not prevent valid documents from being displayed #636

Closed

github-actions bot locked and limited conversation to collaborators Apr 6, 2021

Document labels: use fields other than _id wherever possible #569

Document labels: use fields other than _id wherever possible #569

Conversation

PrashanthCorp commented Apr 28, 2018 • edited Loading

PrashanthCorp Apr 28, 2018 • 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

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

StephenWeatherford commented May 19, 2018

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

PrashanthCorp May 31, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenWeatherford commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrashanthCorp May 31, 2018 • 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

StephenWeatherford commented Jun 12, 2018

PrashanthCorp commented Jun 19, 2018

PrashanthCorp commented Apr 28, 2018 •

edited

Loading

PrashanthCorp Apr 28, 2018 •

edited

Loading

PrashanthCorp May 31, 2018 •

edited

Loading

PrashanthCorp May 31, 2018 •

edited

Loading