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

Improved aggrMethod to accept multiple values #605

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
4 changes: 3 additions & 1 deletion CHANGES_NEXT_RELEASE
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
- Remove: RPM stuff
- Add: aggrMethod accept multiple values separated by comma (#432, partial)
- Add: aggrMethod 'all' to get all the possible aggregations (#432, partial)
- Remove: RPM stuff
8 changes: 5 additions & 3 deletions doc/manuals/aggregated-data-retrieval.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@ The requests for aggregated time series context information can use the followin

- **aggrMethod**: The aggregation method. The STH component supports the following aggregation methods: `max` (maximum
value), `min` (minimum value), `sum` (sum of all the samples) and `sum2` (sum of the square value of all the
samples) for numeric attribute values and `occur` for attributes values of type string. Combining the information
provided by these aggregated methods with the number of samples, it is possible to calculate probabilistic values
such as the average value, the variance as well as the standard deviation. It is a mandatory parameter.
samples) for numeric attribute values and `occur` for attributes values of type string. It accepts multiple values
separated by comma (eg. `aggrMethod=min,max`) to get multiple aggregation method values. Additionally, `aggrMethod=all` can be used
to get all the aggregation method values. Combining the information provided by these aggregated methods with the
number of samples, it is possible to calculate probabilistic values such as the average value, the variance as well
as the standard deviation. It is a mandatory parameter.
- **aggrPeriod**: Aggregation period or resolution. A fixed resolution determines the origin time format and the
possible offsets. It is a mandatory parameter. Possible valid resolution values supported by the STH are: `month`,
`day`, `hour`, `minute` and `second`.
Expand Down
20 changes: 18 additions & 2 deletions lib/database/sthDatabase.js
Original file line number Diff line number Diff line change
Expand Up @@ -760,7 +760,21 @@ function getAggregatedData(data, callback) {
'points.offset': 1,
'points.samples': 1
};
fieldFilter['points.' + aggregatedFunction] = 1;
function findAggregatedFunction(aggregatedFunction) {
let aggregatedFunctionsArray = [];
if (aggregatedFunction.includes('all')) {
aggregatedFunctionsArray = ['min','max','sum','sum2','occur'];
} else {
aggregatedFunctionsArray = aggregatedFunction.split(',');
}
return aggregatedFunctionsArray;
}

const aggregatedFunctions = findAggregatedFunction(aggregatedFunction);

for (let i = 0; i < aggregatedFunctions.length; i++) {
fieldFilter['points.' + aggregatedFunctions[i]] = 1;
}

let originFilter;
if (from && to) {
Expand All @@ -783,7 +797,9 @@ function getAggregatedData(data, callback) {
offset: '$points.offset',
samples: '$points.samples'
};
pushAccumulator[aggregatedFunction] = '$points.' + aggregatedFunction;
for (let i = 0; i < aggregatedFunctions.length; i++) {
pushAccumulator[aggregatedFunctions[i]] = '$points.' + aggregatedFunctions[i];
}

let matchCondition;
switch (sthConfig.DATA_MODEL) {
Expand Down
7 changes: 5 additions & 2 deletions lib/server/sthServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ function doStartServer(host, port, callback) {
reply({ error: 'BadRequest', description: error.output.payload.message }).code(400);
}

const aggList = ['min', 'max', 'sum', 'sum2', 'occur', 'all'];
const joinedAggList = '(' + aggList.join('|') + ')';
const aggRegex = new RegExp('^' + joinedAggList + '(,' + joinedAggList + ')*$');

const config = {
validate: {
headers: sthHeaderValidator,
Expand All @@ -74,8 +78,7 @@ function doStartServer(host, port, callback) {
// prettier-ignore
hOffset: joi.number().integer().greater(-1).optional(),
// prettier-ignore
aggrMethod: joi.string().valid(
'max', 'min', 'sum', 'sum2', 'occur').optional(),
aggrMethod: joi.string().regex(aggRegex).optional(),
// prettier-ignore
aggrPeriod: joi.string().required().valid(
'month', 'day', 'hour', 'minute', 'second').optional(),
Expand Down
139 changes: 111 additions & 28 deletions test/unit/sthTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -912,21 +912,22 @@ function aggregatedDataAvailableSinceDateTest(ngsiVersion, params, done) {
break;
}

let value;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of modifying existing tests, please add new ones. In particular, I'd suggest two new tests cases:

  • Using the 'all' token
  • Using two aggregation methods (whatever) separated by ","

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @fgalan,

I have added the new test cases here.

Previously, only one aggrMethod was there so switch case was working fine. But in case of multiple aggregation method we need to update the code for multiple values separately. The above code change is for util class to accommodate the multiple aggregation methods.

switch (aggrMethod) {
case 'min':
case 'max':
let value, valueSum, valueSum2, valueOccur;
const aggrMethods = aggrMethod.split(',');

for (let i = 0; i < aggrMethods.length; i++) {
if (aggrMethods[i] === 'min' || aggrMethods[i] === 'max') {
value = parseFloat(theEvent.attrValue).toFixed(2);
break;
case 'sum':
value = (events.length * parseFloat(theEvent.attrValue)).toFixed(2);
break;
case 'sum2':
value = (events.length * Math.pow(parseFloat(theEvent.attrValue), 2)).toFixed(2);
break;
case 'occur':
value = events.length;
break;
}
if (aggrMethods[i] === 'sum') {
valueSum = (events.length * parseFloat(theEvent.attrValue)).toFixed(2);
}
if (aggrMethods[i] === 'sum2') {
valueSum2 = (events.length * Math.pow(parseFloat(theEvent.attrValue), 2)).toFixed(2);
}
if (aggrMethods[i] === 'occur') {
valueOccur = events.length;
}
}

if (ngsiVersion === 2) {
Expand Down Expand Up @@ -963,19 +964,37 @@ function aggregatedDataAvailableSinceDateTest(ngsiVersion, params, done) {
events.length
);
if (attrType === 'float') {
expect(
parseFloat(
bodyJSON.value[0].points[sthConfig.FILTER_OUT_EMPTY ? 0 : index][aggrMethod]
).toFixed(2)
).to.equal(value);
for (let j = 0; j < aggrMethods.length; j++) {
if (aggrMethods[j] === 'min' || aggrMethods[j] === 'max') {
expect(
parseFloat(
bodyJSON.value[0].points[sthConfig.FILTER_OUT_EMPTY ? 0 : index][aggrMethods[j]]
).toFixed(2)
).to.equal(value);
}
if (aggrMethods[j] === 'sum') {
expect(
parseFloat(
bodyJSON.value[0].points[sthConfig.FILTER_OUT_EMPTY ? 0 : index][aggrMethods[j]]
).toFixed(2)
).to.equal(valueSum);
}
if (aggrMethods[j] === 'sum2') {
expect(
parseFloat(
bodyJSON.value[0].points[sthConfig.FILTER_OUT_EMPTY ? 0 : index][aggrMethods[j]]
).toFixed(2)
).to.equal(valueSum2);
}
}
} else if (attrType === 'string') {
expect(
parseFloat(
bodyJSON.value[0].points[sthConfig.FILTER_OUT_EMPTY ? 0 : index][aggrMethod][
'just a string'
]
)
).to.equal(value);
).to.equal(valueOccur);
}
done();
}
Expand Down Expand Up @@ -1025,21 +1044,43 @@ function aggregatedDataAvailableSinceDateTest(ngsiVersion, params, done) {
].samples
).to.equal(events.length);
if (attrType === 'float') {
expect(
parseFloat(
bodyJSON.contextResponses[0].contextElement.attributes[0].values[0].points[
sthConfig.FILTER_OUT_EMPTY ? 0 : index
][aggrMethod]
).toFixed(2)
).to.equal(value);
for (let j = 0; j < aggrMethods.length; j++) {
if (aggrMethods[j] === 'min' || aggrMethods[j] === 'max') {
expect(
parseFloat(
bodyJSON.contextResponses[0].contextElement.attributes[0].values[0].points[
sthConfig.FILTER_OUT_EMPTY ? 0 : index
][aggrMethods[j]]
).toFixed(2)
).to.equal(value);
}
if (aggrMethods[j] === 'sum') {
expect(
parseFloat(
bodyJSON.contextResponses[0].contextElement.attributes[0].values[0].points[
sthConfig.FILTER_OUT_EMPTY ? 0 : index
][aggrMethods[j]]
).toFixed(2)
).to.equal(valueSum);
}
if (aggrMethods[j] === 'sum2') {
expect(
parseFloat(
bodyJSON.contextResponses[0].contextElement.attributes[0].values[0].points[
sthConfig.FILTER_OUT_EMPTY ? 0 : index
][aggrMethods[j]]
).toFixed(2)
).to.equal(valueSum2);
}
}
} else if (attrType === 'string') {
expect(
parseFloat(
bodyJSON.contextResponses[0].contextElement.attributes[0].values[0].points[
sthConfig.FILTER_OUT_EMPTY ? 0 : index
][aggrMethod]['just a string']
)
).to.equal(value);
).to.equal(valueOccur);
}
expect(bodyJSON.contextResponses[0].statusCode.code).to.equal('200');
expect(bodyJSON.contextResponses[0].statusCode.reasonPhrase).to.equal('OK');
Expand Down Expand Up @@ -1753,6 +1794,47 @@ function status200Test(ngsiVersion, options, done) {
}
}

/**
* Bad Request 400 status test case
* @param ngsiVersion NGSI version to use. Anything different from 2 (included undefined) means v1
* @param {Object} options Options to generate the URL
* @param {Function} done Callback
*/
function status400Test(ngsiVersion, options, done) {
if (ngsiVersion === 2) {
request(
{
uri: getURL(sthTestConfig.API_OPERATION.READ_V2, options),
method: 'GET',
headers: {
'Fiware-Service': sthConfig.DEFAULT_SERVICE,
'Fiware-ServicePath': sthConfig.DEFAULT_SERVICE_PATH
}
},
function(err, response) {
expect(response.statusCode).to.equal(400);
done();
}
);
} else {
// FIXME: remove the else branch when NGSIv1 becomes obsolete
request(
{
uri: getURL(sthTestConfig.API_OPERATION.READ, options),
method: 'GET',
headers: {
'Fiware-Service': sthConfig.DEFAULT_SERVICE,
'Fiware-ServicePath': sthConfig.DEFAULT_SERVICE_PATH
}
},
function(err, response) {
expect(response.statusCode).to.equal(400);
done();
}
);
}
}

/**
* Test to check that in case of updating a numeric attribute value aggregated data:
* - If the value of the attribute is the same, it is only aggregated once
Expand Down Expand Up @@ -2538,6 +2620,7 @@ module.exports = {
cleanDatabaseSuite,
eventNotificationSuite,
status200Test,
status400Test,
numericAggregatedDataUpdatedTest,
textualAggregatedDataUpdatedTest,
aggregatedDataNonExistentTest,
Expand Down
74 changes: 74 additions & 0 deletions test/unit/sth_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,13 +324,77 @@ describe('sth tests', function() {
})
);

it(
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to add a couple of cases:

  • should respond with 400 - Bad Request if aggMethod is not a valid one (e.g. aggMethod=foo)
  • should respond with 400 - Bad Request if aggMethod mixes a valid one with a not valid one (eg. aggMethod=max,foo)

Btw, what happens if aggMethod mixes all with another method? Eg. aggMethod=all,min? Do we get and error or the "min" part is ignored and that's equivalent to aggMethod=all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test cases in 3b51b4c

If aggMethod mixes all with another method e.g. aggMethod=all,min, then it is equivalent to aggMethod=all ignoring all other parameters.

'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params',
sthTestUtils.status200Test.bind(null, 2, {
aggrMethod: 'min,max',
aggrPeriod: 'second'
})
);

it(
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params',
sthTestUtils.status200Test.bind(null, 2, {
aggrMethod: 'all',
aggrPeriod: 'second'
})
);

it(
'should respond with 400 - Bad Request if aggrMethod is not from [min,max,sum,sum2,occur,all]',
sthTestUtils.status400Test.bind(null, 2, {
aggrMethod: 'foo',
aggrPeriod: 'second'
})
);

it(
'should respond with 400 - Bad Request if aggrMethod are multiple and not from [min,max,sum,sum2,occur,all]',
sthTestUtils.status400Test.bind(null, 2, {
aggrMethod: 'foo,all',
aggrPeriod: 'second'
})
);

it(
'should respond with 200 - OK if aggrMethod and aggrPeriod query params - NGSIv1',
sthTestUtils.status200Test.bind(null, 1, {
aggrMethod: 'min',
aggrPeriod: 'second'
})
);

it(
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params - NGSIv1',
sthTestUtils.status200Test.bind(null, 1, {
aggrMethod: 'min,max',
aggrPeriod: 'second'
})
);

it(
'should respond with 200 - OK if aggrMethod are multiple and aggrPeriod query params - NGSIv1',
sthTestUtils.status200Test.bind(null, 1, {
aggrMethod: 'all',
aggrPeriod: 'second'
})
);

it(
'should respond with 400 - Bad Request if aggrMethod is not from [min,max,sum,sum2,occur,all] - NGSIv1',
sthTestUtils.status400Test.bind(null, 1, {
aggrMethod: 'foo',
aggrPeriod: 'second'
})
);

it(
'should respond with 400 - Bad Request if aggrMethod are multiple and not from [min,max,sum,sum2,occur,all] - NGSIv1',
sthTestUtils.status400Test.bind(null, 1, {
aggrMethod: 'foo,all',
aggrPeriod: 'second'
})
);
});

function eachEventTestSuiteContainer(attrName, attrType, includeTimeInstantMetadata) {
Expand Down Expand Up @@ -390,6 +454,16 @@ describe('sth tests', function() {
'aggregated data retrieval',
sthTestUtils.aggregatedDataRetrievalSuite.bind(null, 'attribute-float', 'float', 'sum2')
);

describe(
'aggregated data retrieval',
sthTestUtils.aggregatedDataRetrievalSuite.bind(null, 'attribute-float', 'float', 'max,sum2')
);

describe(
'aggregated data retrieval',
sthTestUtils.aggregatedDataRetrievalSuite.bind(null, 'attribute-float', 'float', 'all')
);
}

for (let j = 0; j < sthTestConfig.SAMPLES; j++) {
Expand Down
Loading