SQL related improvements (#940)

* Inactive connection originated uuid is now client's uuid

* fix tests

* Change is row set logic

* API doc changes

* Rename variable

* Add comment

* Fix test name

* Fix close test

* Add @ignore to empty modules, adjust comments

* Delete useless internal labels

Author: srknzl

src/sql/SqlError.ts

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+/** @ignore *//** */
 
 import {UUID} from '../core';
 

src/sql/SqlErrorCode.ts

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+/** @ignore *//** */
 
 /**
  * Error codes used in Hazelcast SQL.

src/sql/SqlPage.ts

@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+/** @ignore *//** */
 
 import {SqlColumnType} from './SqlColumnMetadata';
 

src/sql/SqlQueryId.ts

@@ -17,6 +17,7 @@
 import {UUID} from '../core';
 import {UuidUtil} from '../util/UuidUtil';
 import * as Long from 'long';
+/** @ignore *//** */
 
 /**
  * @internal

src/sql/SqlResult.ts

@@ -211,7 +211,7 @@ export class SqlResultImpl implements SqlResult {
 
     isRowSet(): Promise<boolean> {
         return this.executeDeferred.promise.then(() => {
-            return this.updateCount === Long.fromInt(-1);
+            return this.rowMetadata !== null;
         });
     }
 
@@ -242,6 +242,7 @@ export class SqlResultImpl implements SqlResult {
         // Prevent ongoing/future fetch requests
         if (!this.fetchDeferred) {
             this.fetchDeferred = deferredPromise<SqlPage>();
+            this.fetchDeferred.promise.catch(() => {});
         }
         this.fetchDeferred.reject(error);
         // Send the close request.
@@ -255,7 +256,10 @@ export class SqlResultImpl implements SqlResult {
         return this.closeDeferred.promise;
     }
 
-    /** Called when next page of the result is received. */
+    /**
+     * Called when next page of the result is received.
+     * @param page
+     */
     private onNextPage(page: SqlPage) {
         this.currentPage = page;
         this.currentRowCount = page.getRowCount();
@@ -267,7 +271,10 @@ export class SqlResultImpl implements SqlResult {
         }
     }
 
-    /** Called when an error is occurred during SQL execute */
+    /**
+     * Called when an error is occurred during SQL execution.
+     * @param error The wrapped error that can be propagated to the user through executeDeferred.
+     */
     onExecuteError(error: HazelcastSqlException): void {
         // Ignore the error if SQL result is closed.
         if (this.closed) {
@@ -303,18 +310,23 @@ export class SqlResultImpl implements SqlResult {
         }
     }
 
-    /** Called when a execute response is received. */
-    onExecuteResponse(rowMetadata: SqlRowMetadata | null, rowPage: SqlPage, updateCount: Long) {
-        // Ignore the response if SQL result is closed.
+    /**
+     * Called when a execute response is received.
+     * @param rowMetadata  The row metadata. It is null if the response only contains the update count.
+     * @param rowPage The first page of the result. It is null if the response only contains the update count.
+     * @param updateCount The update count.
+     */
+    onExecuteResponse(rowMetadata: SqlRowMetadata | null, rowPage: SqlPage | null, updateCount: Long) {
+        // Ignore the response if the SQL result is closed.
         if (this.closed) {
             return;
         }
 
-        if (rowMetadata !== null) { // Result that including rows
+        if (rowMetadata !== null) { // Result that includes rows
             this.rowMetadata = rowMetadata;
             this.onNextPage(rowPage);
             this.updateCount = Long.fromInt(-1);
-        } else { // Result that including update count
+        } else { // Result that includes update count
             this.updateCount = updateCount;
             this.closed = true;
         }
@@ -333,13 +345,13 @@ export class SqlResultImpl implements SqlResult {
         }
         // Do not start a fetch if the result is already closed
         if (this.closed) {
-            return Promise.reject('Cannot fetch, the result is already closed');
+            return Promise.reject(new IllegalStateError('Cannot fetch, the result is already closed'));
         }
 
         this.fetchDeferred = deferredPromise<SqlPage>();
 
-        this.sqlService.fetch(this.connection, this.queryId, this.cursorBufferSize).then(value => {
-            this.fetchDeferred.resolve(value);
+        this.sqlService.fetch(this.connection, this.queryId, this.cursorBufferSize).then(sqlPage => {
+            this.fetchDeferred.resolve(sqlPage);
             this.fetchDeferred = undefined; // Set fetchDeferred to undefined to be able to fetch again
         }).catch(err => {
             this.fetchDeferred.reject(this.sqlService.toHazelcastSqlException(err, this.connection));
@@ -350,7 +362,6 @@ export class SqlResultImpl implements SqlResult {
 
     /**
      * Checks if there are rows to iterate in a recursive manner, similar to a non-blocking while block
-     * @internal
      */
     private checkHasNext(): Promise<boolean> {
         if (this.currentPosition === this.currentRowCount) {

src/sql/SqlService.ts

@@ -200,7 +200,6 @@ export class SqlServiceImpl implements SqlService {
      *
      * @param sqlStatement
      * @throws RangeError if validation is not successful
-     * @internal
      */
     private static validateSqlStatement(sqlStatement: SqlStatement | null): void {
         if (sqlStatement === null) {
@@ -223,7 +222,6 @@ export class SqlServiceImpl implements SqlService {
      *
      * @param sqlStatementOptions
      * @throws RangeError if validation is not successful
-     * @internal
      */
     private static validateSqlStatementOptions(sqlStatementOptions: SqlStatementOptions): void {
         if (sqlStatementOptions.hasOwnProperty('schema')) {
@@ -253,15 +251,15 @@ export class SqlServiceImpl implements SqlService {
 
     /**
      * Converts an error to HazelcastSqlException and returns it. Used by execute, close and fetch
-     * @internal
      * @param err
      * @param connection
      * @returns {@link HazelcastSqlException}
      */
     toHazelcastSqlException(err: any, connection: Connection) : HazelcastSqlException {
         if (!connection.isAlive()) {
             return new HazelcastSqlException(
-                connection.getRemoteUuid(), SqlErrorCode.CONNECTION_PROBLEM,
+                this.connectionManager.getClientUuid(),
+                SqlErrorCode.CONNECTION_PROBLEM,
                 'Cluster topology changed while a query was executed:' +
                 `Member cannot be reached: ${connection.getRemoteAddress()}`,
                 err
@@ -291,6 +289,7 @@ export class SqlServiceImpl implements SqlService {
 
         const connection = this.connectionRegistry.getRandomConnection(true);
         if (connection === null) {
+            // Either the client is not connected to the cluster, or there are no data members in the cluster.
             throw new HazelcastSqlException(
                 this.connectionManager.getClientUuid(),
                 SqlErrorCode.CONNECTION_PROBLEM,
@@ -378,11 +377,22 @@ export class SqlServiceImpl implements SqlService {
         return this.executeStatement(sqlStatement);
     }
 
+    /**
+     * Sends a close request on a connection for an SQL result using its query id.
+     * @param connection The connection the request will be sent to
+     * @param queryId The query id that defines the SQL result
+     */
     close(connection: Connection, queryId: SqlQueryId): Promise<ClientMessage> {
         const requestMessage = SqlCloseCodec.encodeRequest(queryId);
         return this.invocationService.invokeOnConnection(connection, requestMessage);
     }
 
+    /**
+     * Sends a fetch request on a connection for an SQL result using its query id.
+     * @param connection The connection the request will be sent to
+     * @param queryId The query id that defines the SQL result
+     * @param cursorBufferSize The cursor buffer size associated with SQL fetch request, i.e its page size
+     */
     fetch(connection: Connection, queryId: SqlQueryId, cursorBufferSize: number): Promise<SqlPage> {
         const requestMessage = SqlFetchCodec.encodeRequest(queryId, cursorBufferSize);
         return this.invocationService.invokeOnConnection(connection, requestMessage).then(clientMessage => {

src/util/DatetimeUtil.ts

@@ -13,16 +13,16 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
+/** @ignore *//** */
 
 /**
- Constructs and returns timezone for ISO string from offsetSeconds
- @internal
-
- @param offsetSeconds Offset in seconds, can be negative or positive. must be in valid timezone range [-64800, 64800]. If out of
- this range, the limit values are assumed.
- @throws RangeError if offset seconds is not number
- @return Timezone string, can be 'Z', +hh:mm or -hh:mm
+ * Constructs and returns timezone for ISO string from offsetSeconds
+ * @internal
+ *
+ * @param offsetSeconds Offset in seconds, can be negative or positive. must be in valid timezone range [-64800, 64800]. If out of
+ * this range, the limit values are assumed.
+ * @throws RangeError if offset seconds is not number
+ * @return Timezone string, can be 'Z', +hh:mm or -hh:mm
  */
 export function getTimezoneOffsetFromSeconds(offsetSeconds: number): string {
     if (!Number.isInteger(offsetSeconds)) {
@@ -57,12 +57,12 @@ export function getTimezoneOffsetFromSeconds(offsetSeconds: number): string {
 }
 
 /**
- Parses timezone string and returns offset in seconds
- @internal
-
- @param timezoneString string, can be 'Z', +hh:mm or -hh:mm
- @throws RangeError If timezoneString is invalid
- @return Timezone Offset in seconds, can be negative or positive. must be in valid timezone range [-64800, 64800]
+ * Parses timezone string and returns offset in seconds
+ * @internal
+ *
+ * @param timezoneString string, can be 'Z', +hh:mm or -hh:mm
+ * @throws RangeError If timezoneString is invalid
+ * @return Timezone Offset in seconds, can be negative or positive. must be in valid timezone range [-64800, 64800]
  */
 export function getOffsetSecondsFromTimezoneString(timezoneString: string): number {
     if (typeof timezoneString !== 'string') {
@@ -100,8 +100,9 @@ export function getOffsetSecondsFromTimezoneString(timezoneString: string): numb
 }
 
 /**
- * Give this function integer and it will zero pad to the given length.
  * @internal
+ * Give this function integer and it will zero pad to the given length.
+ *
  * @param value
  * @param length total length after padding
  * @returns Zero padded string
@@ -130,7 +131,9 @@ enum Months {
 }
 
 /**
+ * @internal
  * Returns the string representation of a local date.
+ *
  * @param year Must be between -999999999-999999999
  * @param month Must be between 1-12
  * @param date Must be between 1-31 depending on year and month
@@ -187,6 +190,7 @@ export function getLocalDateString(year: number, month: number, date: number): s
 }
 
 /**
+ * @internal
  * Returns the string representation of a local time.
  *
  * @param hour The hour-of-day to represent, from 0 to 23

test/integration/backward_compatible/sql/ExecuteTest.js

@@ -512,7 +512,7 @@ describe('SqlExecuteTest', function () {
             });
             error1.should.be.instanceof(getHazelcastSqlException());
             error1.code.should.be.eq(getSqlErrorCode().CONNECTION_PROBLEM);
-            error1.originatingMemberId.toString().should.be.eq(member.uuid);
+            error1.originatingMemberId.should.be.eq(client.connectionManager.getClientUuid());
         });
 
         it('should return an error if connection lost - statement', async function () {
@@ -538,7 +538,7 @@ describe('SqlExecuteTest', function () {
             });
             error1.should.be.instanceof(getHazelcastSqlException());
             error1.code.should.be.eq(getSqlErrorCode().CONNECTION_PROBLEM);
-            error1.originatingMemberId.toString().should.be.eq(member.uuid);
+            error1.originatingMemberId.should.be.eq(client.connectionManager.getClientUuid());
         });
 
         it('should return an error if sql is invalid', async function () {

test/integration/backward_compatible/sql/ResultTest.js

@@ -61,8 +61,8 @@ describe('SqlResultTest', function () {
         await someMap.put(0, 1);
         await someMap.put(1, 2);
         await someMap.put(2, 3);
-
-        result = client.getSqlService().execute(`SELECT * FROM ${mapName} WHERE this > ?`, [1]);
+        await someMap.put(3, 4);
+        await someMap.put(4, 5);
     });
 
     after(async function () {
@@ -75,11 +75,15 @@ describe('SqlResultTest', function () {
     });
 
     it('should reject iteration after close()', async function () {
-        await result.close();
-
+        result = client.getSqlService().execute(`SELECT * FROM ${mapName} WHERE this > ?`, [1], {cursorBufferSize: 1});
         const error = await TestUtil.getRejectionReasonOrThrow(async () => {
+            let counter = 0;
             // eslint-disable-next-line no-empty,no-unused-vars
             for await (const row of result) {
+                counter++;
+                if (counter === 2) {
+                    await result.close();
+                }
             }
         });
         error.should.be.instanceof(getHazelcastSqlException());
@@ -89,6 +93,7 @@ describe('SqlResultTest', function () {
     });
 
     it('getters should work', async function () {
+        result = client.getSqlService().execute(`SELECT * FROM ${mapName} WHERE this > ?`, [1]);
         const rowMetadata = await result.getRowMetadata();
         rowMetadata.should.be.instanceof(getSqlRowMetadataImpl());
         rowMetadata.getColumnCount().should.be.eq(2);

test/unit/sql/SqlResult.js

@@ -302,7 +302,7 @@ describe('SqlResultTest', function () {
             }, 100);
         });
 
-        it('should not call onExecuteError and change properties after an error is received', function (done) {
+        it('should call onExecuteError and change properties after an error is received', function (done) {
             const sqlResult = new SqlResultImpl(fakeSqlService, {}, {}, {}, 4096);
             const onExecuteErrorFake = sandbox.replace(sqlResult, 'onExecuteError', sandbox.fake(sqlResult.onExecuteError));
             // simulate a response then call close()