chore: clean up ShellResult usage (#371)

Provide more encapsulation by using a fixed interface for other
packages to work with `shell-api`, without the need to use internal
methods of `shell-api` objects.

- Inside `shell-api`:
  - Consistently use `async [asPrintable]()`, `[resultSource]`,
    `[shellApiType]` (all as symbol properties)
    - These are all non-exported, local symbol properties, i.e.
      not for outside access
  - Remove `_asPrintable`
  - Deprecate `ShellResult.value` in favor of more expressively named
    `.rawValue` and `.printable` (`.value` is very generic, and in
    particular I would have expected it to be the evaluation result
    and not a formatting-ready version of it)
  - Remove `[asShellResult]()`
  - Make `[asPrintable]` methods on the Cursor classed idempotent
- On the API surface of `shell-api`:
  - Remove `asShellResult` and `shellApiType`
  - Provide `getShellApiType()` and `toShellResult()` as the sole APIs
    to turn an evaluation result into a `ShellResult`
  - In particular, this applies for *all* values – no special handling
    is necessary in `shell-evaluator` anymore
  - Remove the special handling for the Java shell that we had
    in the Cursor methods

This is partly in preparation for MONGOSH-237, so that we do not start
introducing calls to half-internal methods like `_asPrintable()`
in more places than we already do.

Author: addaleax

packages/browser-repl/src/components/shell.spec.tsx

@@ -35,7 +35,7 @@ describe('<Shell />', () => {
     elementFocus = sinon.spy(HTMLElement.prototype, 'focus');
 
     fakeRuntime = {
-      evaluate: sinon.fake.returns({ value: 'some result' })
+      evaluate: sinon.fake.returns({ printable: 'some result' })
     };
 
     onOutputChangedSpy = sinon.spy();

packages/browser-repl/src/components/shell.tsx

@@ -108,7 +108,7 @@ export class Shell extends Component<ShellProps, ShellState> {
       outputLine = {
         format: 'output',
         type: result.type,
-        value: result.value
+        value: result.printable
       };
     } catch (error) {
       outputLine = {

packages/browser-repl/src/iframe-runtime/iframe-runtime.spec.ts

@@ -51,7 +51,7 @@ describe('IframeRuntime', () => {
       await other.evaluate('x = 2');
 
       expect(
-        (await runtime.evaluate('x')).value
+        (await runtime.evaluate('x')).printable
       ).to.equal(1);
     });
   });

packages/cli-repl/src/cli-repl.ts

@@ -1,7 +1,7 @@
 /* eslint no-console: 0, no-sync: 0*/
 
 import { CliServiceProvider, NodeOptions, CliOptions } from '@mongosh/service-provider-server';
-import { shellApiType, ShellInternalState } from '@mongosh/shell-api';
+import { getShellApiType, ShellInternalState } from '@mongosh/shell-api';
 import ShellEvaluator from '@mongosh/shell-evaluator';
 import formatOutput, { formatError } from './format-output';
 import { LineByLineInput } from './line-by-line-input';
@@ -227,7 +227,7 @@ class CliRepl {
     this.internalState.setCtx(this.repl.context);
     Object.defineProperty(this.repl.context, 'db', {
       set: (newDb) => {
-        if (newDb === undefined || newDb[shellApiType] !== 'Database') {
+        if (getShellApiType(newDb) !== 'Database') {
           const warn = new MongoshWarning('Cannot reassign \'db\' to non-Database type');
           console.log(warn);
           return;
@@ -357,7 +357,7 @@ class CliRepl {
       return formatOutput({ type: 'Error', value: output });
     }
 
-    return formatOutput(result);
+    return formatOutput({ type: result.type, value: result.printable });
   };
 
   verifyNodeVersion(): void {

packages/java-shell/.gitignore

@@ -1,2 +1,3 @@
 /src/main/resources/js/all-standalone.js
 /build/
+/.gradle/

packages/java-shell/src/main/js/all.js

@@ -4,5 +4,7 @@ const ShellEvaluator = require('../../../../shell-evaluator/').default;
 /** temporal object that is used to access symbols from closures generated by browserify */
 _global = {
     ShellInternalState: ShellApi.ShellInternalState,
-    ShellEvaluator: ShellEvaluator
+    ShellEvaluator: ShellEvaluator,
+    toShellResult: ShellApi.toShellResult,
+    getShellApiType: ShellApi.getShellApiType,
 };

packages/java-shell/src/main/kotlin/com/mongodb/mongosh/MongoShell.kt

@@ -8,9 +8,8 @@ class MongoShell(client: MongoClient) {
 
     fun eval(script: String): MongoShellResult<*> {
         val result = context.unwrapPromise(context.eval(script, "user_script"))
-        val value = result.getMember("value")
-        val type = result.getMember("type")
-        return context.extract(value, if (type.isString) type.asString() else null)
+        val value = result.getMember("rawValue")
+        return context.extract(null, value)
     }
 
     fun close() = context.close()

packages/java-shell/src/main/kotlin/com/mongodb/mongosh/MongoShellContext.kt

@@ -36,6 +36,8 @@ internal class MongoShellContext(client: MongoClient) {
     private val serviceProvider = JavaServiceProvider(client, this)
     private val shellEvaluator: Value
     private val shellInternalState: Value
+    private val toShellResultFn: Value
+    private val getShellApiTypeFn: Value
     private val bsonTypes: BsonTypes
 
     /** Java functions don't have js methods such as apply, bind, call etc.
@@ -51,6 +53,8 @@ internal class MongoShellContext(client: MongoClient) {
         context.removeMember("_global")
         shellInternalState = global.getMember("ShellInternalState").newInstance(serviceProvider)
         shellEvaluator = global.getMember("ShellEvaluator").newInstance(shellInternalState)
+        toShellResultFn = global.getMember("toShellResult")
+        getShellApiTypeFn = global.getMember("getShellApiType")
         val jsSymbol = context["Symbol"]!!
         shellInternalState.invokeMember("setCtx", context)
         initContext(context, jsSymbol)
@@ -161,13 +165,31 @@ internal class MongoShellContext(client: MongoClient) {
         }
     }
 
-    fun extract(v: Value, type: String? = null): MongoShellResult<*> {
+    fun getShellApiType(rawValue: Value): String? {
+        val rawType = getShellApiTypeFn.execute(rawValue)
+        return if (rawType.isString) rawType.asString() else null
+    }
+
+    fun toShellResult(rawValue: Value): Value {
+        return unwrapPromise(toShellResultFn.execute(rawValue));
+    }
+
+    fun extract(printable: Value?, rawValue: Value? = null): MongoShellResult<*> {
+        val v: Value
+        val type: String?
+        if (printable == null) {
+            type = getShellApiType(rawValue as Value)
+            v = toShellResult(rawValue)["printable"]!!
+        } else {
+            type = null
+            v = printable
+        }
+
         return when {
-            v.instanceOf("Promise") -> extract(unwrapPromise(v))
             type == "Help" -> extract(v["attr"]!!)
-            type == "Cursor" -> FindCursorResult(FindCursor<Any?>(v, this))
+            type == "Cursor" -> FindCursorResult(FindCursor<Any?>(rawValue, this))
             // document with aggregation explain result also has type AggregationCursor, so we need to make sure that value contains cursor
-            type == "AggregationCursor" && v.hasMember("_cursor") -> AggregationCursorResult(AggregationCursor<Any?>(v, this))
+            type == "AggregationCursor" && rawValue!!.hasMember("_cursor") -> AggregationCursorResult(AggregationCursor<Any?>(rawValue, this))
             type == "InsertOneResult" -> InsertOneResult(v["acknowledged"]!!.asBoolean(), v["insertedId"]!!.asString())
             type == "DeleteResult" -> DeleteResult(v["acknowledged"]!!.asBoolean(), v["deletedCount"]!!.asLong())
             type == "UpdateResult" -> {
@@ -345,4 +367,4 @@ private val DATE_FORMATTER = DateTimeFormatterBuilder()
         .appendLiteral('Z')
         .optionalEnd()
         .optionalEnd()
-        .toFormatter()
+        .toFormatter()

packages/java-shell/src/main/kotlin/com/mongodb/mongosh/result/AggregationCursor.kt

@@ -17,7 +17,7 @@ class AggregationCursor<out T> internal constructor(private var cursor: Value?,
 
     override fun _asPrintable(): String {
         val (cursor, context) = checkClosed()
-        return context.extract(cursor.invokeMember("_asPrintable"))._asPrintable()
+        return context.extract(context.toShellResult(cursor).getMember("printable"))._asPrintable()
     }
 
     override fun close() {
@@ -33,4 +33,4 @@ class AggregationCursor<out T> internal constructor(private var cursor: Value?,
         if (cursor == null || context == null) throw IllegalStateException("Cursor has already been closed")
         return cursor to context
     }
-}
+}

packages/java-shell/src/main/kotlin/com/mongodb/mongosh/result/FindCursor.kt

@@ -22,7 +22,7 @@ class FindCursor<out T> internal constructor(private var cursor: Value?, private
 
     override fun _asPrintable(): String {
         val (cursor, context) = checkClosed()
-        return context.extract(cursor.invokeMember("_asPrintable"))._asPrintable()
+        return context.extract(context.toShellResult(cursor).getMember("printable"))._asPrintable()
     }
 
     override fun close() {
@@ -38,4 +38,4 @@ class FindCursor<out T> internal constructor(private var cursor: Value?, private
         if (cursor == null || context == null) throw IllegalStateException("Cursor has already been closed")
         return cursor to context
     }
-}
+}