Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SNOW-1926267: Fix promise rejecting for file upload errors #1008

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions lib/connection/result/row_stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ function RowStream(statement, context, options) {
} else if (context.isFetchingResult) {
// if we're still fetching the result, wait for the operation to complete
context.on('statement-complete', init);
} else if (context.result || isStatementErrorFatal(context.resultError)) {
} else if (context.result || isStatementErrorFatal(context)) {
// if we have a result or a fatal error, call init() in the next tick of
// the event loop
process.nextTick(init);
Expand Down Expand Up @@ -295,12 +295,16 @@ Util.inherits(RowStream, Readable);
/**
* Determines if a statement error is fatal.
*
* @param {Error} error
*
* @returns {Boolean}
* @param context
*/
function isStatementErrorFatal(error) {
return Errors.isOperationFailedError(error) && error.sqlState;
function isStatementErrorFatal(context) {
const error = context.resultError;
return (Errors.isOperationFailedError(error) && error.sqlState) || isFileUploadError(error, context.type);
}

function isFileUploadError(error, contextType) {
return error && contextType === 'FILE_PRE_EXEC';
}

/**
Expand Down
51 changes: 31 additions & 20 deletions lib/connection/statement.js
Original file line number Diff line number Diff line change
Expand Up @@ -813,26 +813,7 @@ function FileStatementPreExec(
* @param {Object} body
*/
context.onStatementRequestSucc = async function (body) {
context.fileMetadata = body;

const fta = new FileTransferAgent(context);
await fta.execute();

// build a result from the response
const result = fta.result();

// init result and meta
body.data.rowset = result.rowset;
body.data.returned = body.data.rowset.length;
body.data.rowtype = result.rowtype;
body.data.parameters = [];

context.result = new Result({
response: body,
statement: this,
services: context.services,
connectionConfig: context.connectionConfig
});
await executeFileTransferRequest(context, body, this);
};

/**
Expand All @@ -858,6 +839,36 @@ function FileStatementPreExec(
sendRequestPreExec(context, context.onStatementRequestComp);
}

async function executeFileTransferRequest(context, body, statement, fileTransferAgent) {
context.fileMetadata = body;

const fta = fileTransferAgent ?? new FileTransferAgent(context); await fta.execute();

try {
// build a result from the response
const result = fta.result();

// init result and meta
body.data = {
rowset: result.rowset,
returned: result.rowset.length,
rowtype: result.rowtype,
parameters: [],
};

context.result = new Result({
response: body,
statement: statement,
services: context.services,
connectionConfig: context.connectionConfig
});
} catch (error) {
context.resultError = error;
}
}

exports.executeFileTransferRequest = executeFileTransferRequest;

Util.inherits(FileStatementPreExec, BaseStatement);

/**
Expand Down
14 changes: 12 additions & 2 deletions lib/file_transfer_agent/file_transfer_agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,13 @@ function FileTransferAgent(context) {

if (results) {
for (const meta of results) {
if (meta['resultStatus'] === 'ERROR') {
errorDetails = meta['errorDetails'];
if (!errorDetails) {
errorDetails = `Unknown error during PUT of file: ${meta['srcFilePath']}`;
}
throw new Error(errorDetails);
}
if (meta['srcCompressionType']) {
srcCompressionType = meta['srcCompressionType']['name'];
} else {
Expand All @@ -227,8 +234,8 @@ function FileTransferAgent(context) {

errorDetails = meta['errorDetails'];

srcFileSize = meta['srcFileSize'].toString();
dstFileSize = meta['dstFileSize'].toString();
srcFileSize = meta['srcFileSize'];
dstFileSize = meta['dstFileSize'];

rowset.push([
meta['srcFileName'],
Expand Down Expand Up @@ -334,6 +341,9 @@ function FileTransferAgent(context) {
continue;
}
results.push(result);
if (result['resultStatus'] === resultStatus.ERROR) {
break;
}
index += 1;
if (INJECT_WAIT_IN_PUT > 0) {
await new Promise(resolve => setTimeout(resolve, INJECT_WAIT_IN_PUT));
Expand Down
17 changes: 17 additions & 0 deletions test/unit/connection/statement_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -411,3 +411,20 @@ describe('Statement.fetchResult()', function () {
it(testCase.name, createItCallback(testCase));
}
});

it('Statement file transfer error', async function () {
const mockFta = {
execute: async function () {
return null;
},
result: function () {
throw new Error('some file transfer error');
}
};
const context = {};
const body = {
'data': {},
};
await Statement.executeFileTransferRequest(context, body, null, mockFta);
assert.strictEqual(context.resultError.message, 'some file transfer error');
});