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.

Sign up for GitHub

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

Jump to bottom

Fix error handling for getColumn when table does not exist #57883

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Fix error handling for getColumn when table does not exist #57883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions apps/src/storage/firebaseStorage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1241,9 +1241,13 @@ FirebaseStorage.getColumn = function (
tableName,
{},
records => {
let columnValues = [];
records.forEach(row => columnValues.push(row[columnName]));
onSuccess(columnValues);
if (records === null) {
onSuccess(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null check here avoids exception on null.forEach and passes null back to handleGetColumn here: https://github.com/code-dot-org/code-dot-org/blob/cassi/applab-getcol-onfailure-fix/apps/src/applab/commands.js#L1877-L1878

} else {
let columnValues = [];
records.forEach(row => columnValues.push(row[columnName]));
Comment on lines +1247 to +1248
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can just be a map.

const columnValues = records.map(row => row[columnName])

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, also just as another note in case anyone looks to this code again...

even if it's not switched to the map, it should still be const columnValues = []. It doesn't need to be a let.

const only prevents you from re-assigning the variable itself (e.g., columnValues = 'foo' later on wouldn't be allowed), but you can still manipulate the object itself. JavaScript is weird. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate this comment and will keep it in mind going forward. Because this is in the critical path for the AP CSP Create task though, I want to keep the change as scoped as possible and only touch the "records is null" fork of the code path. So I'll leave it as is for this PR. I would follow-up and change it after Create task next month, but since this file will be deleted in a couple months anyway, I'm going to skip the overhead. BUT I will remember this fact about const, and how much cleaner map is than the forEach push song and dance! Thank you!!

onSuccess(columnValues);
}
},
onError
);
Expand Down
75 changes: 75 additions & 0 deletions apps/test/integration/levelSolutions/applab1/ec_data_blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,81 @@ export default {
},
},

{
description:
'Data getColumn returns correct console message for nonexistant table',
editCode: true,
xml: `var names = getColumn('nonexistant table', 'name');`,

runBeforeClick(assert) {
// add a completion on timeout since this is a freeplay level
tickWrapper.runOnAppTick(Applab, 200, () => {
Applab.onPuzzleComplete();
});
},
customValidator(assert) {
// Error text includes correct info
const debugOutput = document.getElementById('debug-output');
assert.equal(
String(debugOutput.textContent).startsWith('ERROR'),
true,
`log message contains error: ${debugOutput.textContent}`
);
assert.equal(
String(debugOutput.textContent).endsWith(
"but that table doesn't exist in this app"
),
true,
`log message contains correct info about nonexistant table: ${debugOutput.textContent}`
);
return true;
},
expected: {
result: true,
testResult: TestResults.FREE_PLAY,
},
},

{
description:
'Data getColumn returns correct console message for nonexistant column',
editCode: true,
xml: `
createRecord("mytable", {name:'Alice'}, function(record) {
createRecord("mytable", {name: 'Bob'}, function (record) {
var names = getColumn('mytable', 'nonexistant column');
});
});`,

runBeforeClick(assert) {
// add a completion on timeout since this is a freeplay level
tickWrapper.runOnAppTick(Applab, 200, () => {
Applab.onPuzzleComplete();
});
},
customValidator(assert) {
// Error text includes correct info
const debugOutput = document.getElementById('debug-output');
assert.equal(
String(debugOutput.textContent).startsWith('ERROR'),
true,
`log message contains error: ${debugOutput.textContent}`
);
assert.equal(
String(debugOutput.textContent).endsWith(
"but that column doesn't exist. "
),
true,
`log message contains correct info about nonexistant column: ${debugOutput.textContent}`
);
return true;
},
expected: {
result: true,
testResult: TestResults.FREE_PLAY,
},
},

{
description: 'Data createRecord again to confirm mock is reset',
editCode: true,
Expand Down
91 changes: 91 additions & 0 deletions apps/test/unit/storage/FirebaseStorageTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1172,6 +1172,97 @@ describe('FirebaseStorage', () => {
});
});

describe('getColumn', () => {
it('can get column when the column exists', done => {
const csvData =
'id,name,age,male\n' +
'4,alice,7,false\n' +
'5,bob,8,true\n' +
'6,charlie,9,true\n';
Comment on lines +1178 to +1181
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, just a non-mandatory suggestion, but string concatenation and especially with embedded newlines is a little twitchy and easy to have errors (e.g., by forgetting the newline).

I'd suggest a set of lists and a join on "\n"

const csvData = [
  'id,name,age,male',
  '4,alice,7,false',
  '5,bob,8,true',
  '6,charlie,9,true'].join("\n");

Reads a little cleaner to me. We could also take this to an extra level and join the subarrays on commas, but that feels like overkill unless we needed to be dynamic about the record delimeter.

Also, as a side note - backtick quotes do allow embedded newlines, but since this is formatted data I'd lean against using them in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted, and there are a lot of things I could clean up in this file but am avoiding that rabbit hole since this code will be going away with the rest of firebase. Thank you!!

const expectedColumnValues = ['alice', 'bob', 'charlie'];

FirebaseStorage.importCsv(
'mytable',
csvData,
() => {
FirebaseStorage.getColumn('mytable', 'name', onSuccess, error => {
throw error;
});
},
error => {
throw error;
}
);
function onSuccess(columnValues) {
expect(columnValues).to.deep.equal(expectedColumnValues);
done();
}
});

it('can read cols from a current table', done => {
const tableData = {
1: '{"id":1,"name":"alice","age":7,"male":false}',
2: '{"id":2,"name":"bob","age":8,"male":true}',
3: '{"id":3,"name":"charlie","age":9,"male":true}',
};
getSharedDatabase()
.child('counters/tables/mytable')
.set({lastId: 3, rowCount: 3});
getSharedDatabase()
.child('storage/tables/mytable/records')
.set(tableData);

getProjectDatabase().child('current_tables/mytable').set(true);

const expectedColumnValues = ['alice', 'bob', 'charlie'];

FirebaseStorage.getColumn(
'mytable',
'name',
colValues => {
expect(colValues).to.deep.equal(expectedColumnValues);
done();
},
err => {
throw 'error';
}
);
});

it('returns [] for a table with no rows', done => {
FirebaseStorage.createTable(
'emptytable',
() => {
FirebaseStorage.getColumn(
'emptytable',
'column',
onSuccess,
error => {
throw error;
}
);
},
error => {
throw error;
}
);
function onSuccess(colValues) {
expect(colValues).to.deep.equal([]);
done();
}
});

it('returns null for a non-existent table', done => {
FirebaseStorage.getColumn('notATable', 'column', onSuccess, error => {
throw error;
});
function onSuccess(colValues) {
expect(colValues).to.equal(null);
done();
}
});
});

describe('readRecords', () => {
it('can read a table with rows', done => {
const csvData =
Expand Down