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

Issue 1217 - csv upload validation #1403

Open
wants to merge 18 commits into
base: development
Choose a base branch
from
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
209 changes: 203 additions & 6 deletions src/server/services/csvPipeline/uploadMeters.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,18 @@ const Meter = require('../../models/Meter');
const readCsv = require('../pipeline-in-progress/readCsv');
const Unit = require('../../models/Unit');
const { normalizeBoolean } = require('./validateCsvUploadParams');
const { type } = require('os');
Copy link
Member

Choose a reason for hiding this comment

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

I think this include may have been accidentally added as I don't think it is used.


/**
* Middleware that uploads meters via the pipeline. This should be the final stage of the CSV Pipeline.
* @param {express.Request} req
* @param {express.Response} res
* @param {express.Request} req
* @param {express.Response} res
* @param {filepath} filepath Path to meters csv file.
* @param conn Connection to the database.
*/
async function uploadMeters(req, res, filepath, conn) {
const temp = (await readCsv(filepath)).map(row => {
// The Canonical structure of each row in the Meters CSV file is the order of the fields
// The Canonical structure of each row in the Meters CSV file is the order of the fields
// declared in the Meter constructor. If no headerRow is provided (i.e. headerRow === false),
// then we assume that the uploaded CSV file follows this Canonical structure.

Expand All @@ -41,21 +42,71 @@ async function uploadMeters(req, res, filepath, conn) {
try {
for (let i = 0; i < meters.length; i++) {
let meter = meters[i];
//validation for boolean values
validateBooleanFields(meter, i);

// Validate min and max values
validateMinMaxValues(meter, i);

// First verify GPS is okay
// This assumes that the sixth column is the GPS as order is assumed for now in a GPS file.
const gpsInput = meter[6];
// Skip if undefined.
if (gpsInput) {
// Verify GPS is okay values
if (!isValidGPSInput(gpsInput)) {
let msg = `For meter ${meter[0]} the gps coordinates of ${gpsInput} are invalid`;
let msg = `For meter ${meter[0]} the gps coordinates of ${gpsInput} are invalid.`;
throw new CSVPipelineError(msg, undefined, 500);
}
// Need to reverse latitude & longitude because standard GPS gives in that order but a GPSPoint for the
// DB is longitude, latitude.
meter[6] = switchGPS(gpsInput);
}

// Verify area unit - have to parse float to ensure it can read in integer numbers like "33"
const areaInput = parseFloat(meter[9]);
Copy link
Member

Choose a reason for hiding this comment

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

Just noting you parseFloat before the check for being a number and if it is undefined.

if (areaInput) {
Copy link
Member

Choose a reason for hiding this comment

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

This check can fail in certain circumstances. I tested and 0 (zero) and -1 returns false so the area check does not happen. I also notice that 0.1 or any value with a decimal part does not work but it should. Finally, this allowed letters for numbers to pass the test. This may impact other tests you added. I did not try a fix so let me know if you have issues in resolving.

if (!isValidArea(areaInput)){
Copy link
Member

Choose a reason for hiding this comment

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

This is a small point but here and a few other lines don't have spaces: ){ vs ) }. Formatting with VSC should clear this up. I can do it if you prefer.

let msg = `For meter ${meter[0]} the area entry of ${areaInput} is invalid. Area must be a number greater than 0.`;
throw new CSVPipelineError(msg, undefined, 500);
}
}

const timeSortValue = meter[17];
if (timeSortValue) {
if (!isValidTimeSort(timeSortValue)){
let msg = `For meter ${meter[0]} the time sort ${timeSortValue} is invalid. Valid options are increasing or decreasing.`;
throw new CSVPipelineError(msg, undefined, 500);
}
}

const timezone = meter[5];
if (timezone){
if (!isValidTimeZone(timezone)){
let msg = `For meter ${meter[0]}, ${timeSortValue} is not a valid time zone.`;
Copy link
Member

Choose a reason for hiding this comment

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

I think timeSortValue should be for time zone instead.

throw new CSVPipelineError(msg, undefined, 500);
}
}

// Verify area unit provided
const areaUnitString = meter[25];
if (areaUnitString) {
if (!isValidAreaUnit(areaUnitString)){
let msg = `For meter ${meter[0]} the area unit of ${areaUnitString} is invalid. Unit must be feet, meters, or none.`;
throw new CSVPipelineError(msg, undefined, 500);
}
}

// Verify meter type
const meterTypeString = meter[4];
if (meterTypeString) {
if (!isValidMeterType(meterTypeString)){
let msg = `For meter ${meter[0]} the meter type of ${meterTypeString} is invalid. Valid types include:
egauge, mamac, metasys, obvius, and other. `;
throw new CSVPipelineError(msg, undefined, 500);
}
}

// Process unit.
const unitName = meter[23];
const unitId = await getUnitId(unitName, Unit.unitType.METER, conn);
Expand Down Expand Up @@ -109,7 +160,7 @@ async function uploadMeters(req, res, filepath, conn) {
throw new CSVPipelineError(
`Meter name of \"${meter[0]}\" got database error of: ${error.message}`, undefined, 500);
}
);
);
}
}
} catch (error) {
Expand Down Expand Up @@ -152,6 +203,78 @@ function switchGPS(gpsString) {
return (array[1] + ',' + array[0]);
}

/**
* Checks if the area provided is a number and if it is larger than zero.
* @param areaInput the provided area for the meter
* @returns true or false
*/
function isValidArea(areaInput) {
// must be a number and must be non-negative
if (Number.isInteger(areaInput) && areaInput > 0){
return true;
} else {
return false;
}
}

/**
* Checks if the area unit provided is an option
* @param areaUnit the provided area for the meter
* @returns true or false
*/
function isValidAreaUnit(areaUnit) {
const validTypes = ['feet', 'meters', 'none'];
Copy link
Member

Choose a reason for hiding this comment

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

I think using the standard enum would be better. See src/server/models/Unit.js for areaUnitType. This will allow it to adjust if the allowed values are changed. Also, the error msg above with these values should be constructed by looping over the enum. Also for potential future changes the comment(s) listing the values could be updated about the enum rather than specifics.

// must be one of the three values
if (validTypes.includes(areaUnit)){
return true;
} else {
return false;
}
}

/**
* Checks if the time sort value provided is accurate (should be increasing or decreasing)
* @param timeSortValue the provided time sort
* @returns true or false
*/
function isValidTimeSort(timeSortValue) {
// must be one of the three values
if (timeSortValue == 'increasing' || timeSortValue == 'decreasing'){
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above about enum, see src/server/services/csvPipeline/validateCsvUploadParams.js for MeterTimeSortTypesJS.

return true;
} else {
return false;
}
}

/**
* Checks if the meter type provided is one of the 5 options allowed when creating a meter.
* @param meterTypeString the string for the meter type
* @returns true or false
*/
function isValidMeterType(meterTypeString) {
const validTypes = ['egauge', 'mamac', 'metasys', 'obvius', 'other'];
Copy link
Member

Choose a reason for hiding this comment

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

Same enum and see src/server/models/Meter.js for `Meter.type``.

if (validTypes.includes(meterTypeString)){
return true;
} else {
return false;
}
}

/**
* Checks the provided time zone and if it is a real time zone.
* @param zone the provided time zone from the csv
* @returns true or false
*/
function isValidTimeZone(zone) {
// check against the built in timezones, must use a try catch since it does not return a boolean
try {
new Intl.DateTimeFormat(undefined, {timeZone : zone});
Copy link
Member

Choose a reason for hiding this comment

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

The src/client/app/components/TimeZoneSelect.tsx front-end component uses moment.tz.names() to get the name choices. It would be good to be consistent and use that here if we can. It may mean the try/catch is not needed.

return true;
} catch (e) {
return false;
}
}

/**
* Return the id associated with the given unit's name.
* If the unit's name is invalid or its type is different from expected type, return null.
Expand All @@ -170,4 +293,78 @@ async function getUnitId(unitName, expectedUnitType, conn) {
return unit.id;
}

module.exports = uploadMeters;

Comment on lines 295 to +296
Copy link
Member

Choose a reason for hiding this comment

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

Maybe remove second blank line.

/**
* Validates all boolean-like fields for a given meter row.
* @param {Array} meter - A single row from the CSV file.
* @param {number} rowIndex - The current row index for error reporting.
*/
function validateBooleanFields(meter, rowIndex) {
// all inputs that involve a true or false all bieng validated together.
Copy link
Member

Choose a reason for hiding this comment

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

bieng ->> being

const booleanFields = {
2: 'enabled',
3: 'displayable',
10: 'cumulative',
11: 'reset',
18: 'end only',
32: 'disableChecks'
};

// this array has values which may be left empty
const booleanUndefinedAcceptable = [
'cumulative', 'reset', 'end only', 'disableChecks'
];

for (const [index, name] of Object.entries(booleanFields)) {
let value = meter[index];

// allows upper/lower case.
if ((value === '' || value === undefined) && booleanUndefinedAcceptable.includes(name)) {
// skip if the value is undefined
continue;
} else {
if (typeof value === 'string') {
value = value.toLowerCase();
}
}

// Validates read values to either false or true
if (value !== 'true' && value !== 'false' && value !== true && value !== false
&& value !== 'yes' && value !== 'no') {
throw new CSVPipelineError(
`Invalid input for '${name}' in row ${rowIndex + 1}: "${meter[index]}". Expected 'true' or 'false'.`,
undefined,
500
);
}
}
}

function validateMinMaxValues(meter, rowIndex) {
const minValue = Number(meter[27]);
const maxValue = Number(meter[28]);

if (isNaN(minValue)){
// do nothing, pass it through
} else if (isNaN(minValue) || minValue < -9007199254740991 || minValue >= maxValue){
Copy link
Member

Choose a reason for hiding this comment

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

I think OED is allowing minValue = maxValue. This applies below.

This did raise the question in my mind that the maxValue is used before it is validated. Should they be checked for isNan, isNumber first, then min/max value and finally if min > max?

throw new CSVPipelineError(
`Invalid min/max values in row ${rowIndex + 1}: min="${meter[27]}", max="${meter[28]}". ` +
`Min or/and max must be a number larger than -9007199254740991, and less then 9007199254740991, and min must be less than max.`,
undefined,
500
);
}

if (isNaN(maxValue)){
// do nothing, pass it through
} else if (isNaN(maxValue) || maxValue > 9007199254740991 || minValue >= maxValue){
throw new CSVPipelineError(
`Invalid min/max values in row ${rowIndex + 1}: min="${meter[27]}", max="${meter[28]}". ` +
`Min or/and max must be a number larger than -9007199254740991, and less then 9007199254740991, and min must be less than max.`,
undefined,
500
);
}
}

module.exports = uploadMeters;
Loading