Skip to content

[O2B-1602] Shared BeamTypesDto should return split input rather than simply validation #2202

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

Open
graduta wants to merge 8 commits into from
Open

[O2B-1602] Shared BeamTypesDto should return split input rather than simply validation #2202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1e8d30e
Fix spelling mistake
graduta Jul 31, 2026
ded1c12
Update beam type dto to split and return array of values
graduta Jul 31, 2026
15fdc9c
Remove unused utility and update tests
graduta Jul 31, 2026
b9334a3
Fix API tests
graduta Jul 31, 2026
67dd8a3
Push also changes related to Runs as are using the BeamTypesDto
graduta Jul 31, 2026
33cf287
FIx GetAllRuns tests
graduta Jul 31, 2026
a35a5e8
Provide more details to beam type format
graduta Aug 1, 2026
11a0a33
Fix test for error message
graduta Aug 1, 2026
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
38 changes: 30 additions & 8 deletions lib/domain/dtos/common/BeamTypeDto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,34 @@
*/

const Joi = require('joi');
const { validateBeamTypes, BEAM_TYPE_INVALID } = require('../../../utilities/beamTypeUtils');
const { CustomJoi } = require('../CustomJoi.js');

@isaachilly isaachilly Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I understand this was not introduced in this PR, so not to be implemented here, but would you agree that CustomJoi is a rather nondescript name and should be changed to something like `StringArrayJoi? If so, I will make a note and then the change.

@graduta graduta Aug 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I do not believe it should be changed to "StringArrayJoi" because the name CustomJoi refers to the wrapper defined in that file.
If you look at the implementation of the wrapper, you will see it defines the method stringArray which is being called in this file.
Thus, to me:

  • "CustomJoi" - informs the developer that it is a custom wrapper around the Joi library
  • "CustomJoi.stringArray()" - it is the call that returns the string array


exports.BeamTypesDto = Joi.string()
.trim()
.custom(validateBeamTypes)
.messages({
[BEAM_TYPE_INVALID]: '{{#message}}',
'string.base': 'Beam type must be a string',
});
/**
* @typedef {string[]} BeamTypesDto
* @description An array of beam types, each represented as a string.
* Each beam type must be a string with a minimum length of 2 characters and a maximum length of 15 characters.
* The string must match patterns such as "PROTON - PROTON", "NE10 - NE10", where the two parts are separated by a hyphen and optional spaces.
*
* RUN3 has the following beam types:
* "PROTON - PROTON"
* "NE10 - NE10"
* "O8 - O8"
* "PB82 - PB82"
* "PROTON - O8"
* "PROTON - PROTON"
*
* @example
* const beamTypes = ["PROTON - PROTON", "NE10 - NE10"];
*/
exports.BeamTypesDto = CustomJoi.stringArray()
.items(Joi.string()
.trim()
.min(2)

@isaachilly isaachilly Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this minimum check necessary as the pattern enforces >2 chars with char dash char?

@graduta graduta Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

While the pattern will enforce that, I think it is also important to be self-explanatory when writing code, so that it is easy to maintain it.
Moreover, regexes are in general slow, so in this case a length-based match may be faster for scenarios in which we receive thousands of requests within a second.

.max(15)
.pattern(/^[A-Za-z0-9]+ ?- ?[A-Za-z0-9]+$/)
.messages({

@isaachilly isaachilly Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The custom validator was the only thing raising BEAM_TYPE_INVALID error so I believe this is now dead message handler code.

Supported by the fact the test/api/runs.test.js LN196 test has been changed to assert the default JOI message format.

Do you agree?

@graduta graduta Aug 1, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You are correct. I have modified the DTO to have more specific error messages now

'string.base': 'Beam type must be a string',
'string.min': 'Beam type must be at least 2 characters long',
'string.max': 'Beam type must be at most 15 characters long',
'string.pattern.base': 'Beam type must look like "PROTON - PROTON", "NE10 - NE10", etc.',
}));
3 changes: 1 addition & 2 deletions lib/usecases/lhcFill/GetAllLhcFillsUseCase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ class GetAllLhcFillsUseCase {
}

if (beamTypes) {
const beamTypesArray = beamTypes.split(',');
queryBuilder.where('beamType').oneOf(beamTypesArray);
queryBuilder.where('beamType').oneOf(beamTypes);
}

if (schemeName) {
Expand Down
3 changes: 1 addition & 2 deletions lib/usecases/run/GetAllRunsUseCase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,9 @@ class GetAllRunsUseCase {
}

if (beamTypes) {
const beamTypesList = splitStringToStringsTrimmed(beamTypes, SEARCH_ITEMS_SEPARATOR);
filteringQueryBuilder.include({
association: 'lhcFill',
where: { beamType: { [Op.in]: beamTypesList } },
where: { beamType: { [Op.in]: beamTypes } },
required: true,
});
}
Expand Down
47 changes: 0 additions & 47 deletions lib/utilities/beamTypeUtils.js
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions lib/utilities/stringUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ const snakeToPascal = (snake) => ucFirst(snakeToCamel(snake));
* Split the received string to an array of trimmed strings.
* Boolean trick: https://michaeluloth.com/javascript-filter-boolean/
* @param {string} stringCollection String containing other strings withing split by seperator.
* @param {string} stringSeperator Used to seperate the stringCollection.
* @param {string} stringSeparator Used to seperate the stringCollection.
*/
const splitStringToStringsTrimmed = (stringCollection, stringSeperator = ',') => stringCollection.split(stringSeperator)
const splitStringToStringsTrimmed = (stringCollection, stringSeparator = ',') => stringCollection.split(stringSeparator)
.map((string) => string.trim())
.filter(Boolean);

Expand Down
2 changes: 1 addition & 1 deletion test/api/lhcFills.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -771,7 +771,7 @@ module.exports = () => {

const { errors: [error] } = res.body;
expect(error.title).to.equal('Invalid Attribute');
expect(error.detail).to.equal('"query.filter.beamTypes" is not allowed to be empty');
expect(error.detail).to.equal('"query.filter.beamTypes[0]" is not allowed to be empty');
done();
});
});
Expand Down
10 changes: 5 additions & 5 deletions test/api/runs.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ module.exports = () => {
});

it('should successfully filter with multiple beamTypes', async () => {
const beamTypes = ['p-p', 'Pb-Pb'];
const response = await request(server).get(`/api/runs?filter[beamTypes]=${beamTypes.join(',')}`);
const beamTypes = 'p-p,Pb-Pb';
const response = await request(server).get(`/api/runs?filter[beamTypes]=${beamTypes}`);

expect(response.status).to.equal(200);
const { data: runs } = response.body;
Expand All @@ -185,15 +185,15 @@ module.exports = () => {
});

it('should return 400 if beamTypes filter has the incorrect format', async () => {
const beamTypeString = 'DOES NOT EXIST';
const response = await request(server).get(`/api/runs?filter[beamTypes]=${beamTypeString}`);
const beamTypes = 'DOES NOT EXIST';
const response = await request(server).get(`/api/runs?filter[beamTypes]=${beamTypes}`);

expect(response.status).to.equal(400);

const { errors: [error] } = response.body;

expect(error.title).to.equal('Invalid Attribute');
expect(error.detail).to.equal(`Invalid beam type format: ${beamTypeString}`);
expect(error.detail).to.equal(`Beam type must look like "PROTON - PROTON", "NE10 - NE10", etc.`);
});

it('should return 400 if beamModes filter has the incorrect format', async () => {
Expand Down
6 changes: 3 additions & 3 deletions test/lib/usecases/lhcFill/GetAllLhcFillsUseCase.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ module.exports = () => {
})

it('should only contain specified beam type, {p-p}', async () => {
getAllLhcFillsDto.query = { filter: { beamTypes: 'p-p' } };
getAllLhcFillsDto.query = { filter: { beamTypes: ['p-p'] } };
const { lhcFills } = await new GetAllLhcFillsUseCase().execute(getAllLhcFillsDto)

expect(lhcFills).to.be.an('array').and.lengthOf(2)
Expand All @@ -290,7 +290,7 @@ module.exports = () => {
it('should only contain specified beam types, {p-p, PROTON-PROTON, Pb-Pb}', async () => {
const beamTypes = ['p-p', 'PROTON-PROTON', 'Pb-Pb']

getAllLhcFillsDto.query = { filter: { beamTypes: beamTypes.join(',') } };
getAllLhcFillsDto.query = { filter: { beamTypes: beamTypes } };
const { lhcFills } = await new GetAllLhcFillsUseCase().execute(getAllLhcFillsDto)

expect(lhcFills).to.be.an('array').and.lengthOf(4)
Expand All @@ -302,7 +302,7 @@ module.exports = () => {
it('should ignore unknown beam types, {p-p, Hello-world, Pb-Pb}', async () => {
const beamTypes = ['p-p', 'Hello-world', 'Pb-Pb']

getAllLhcFillsDto.query = { filter: { beamTypes: beamTypes.join(',') } };
getAllLhcFillsDto.query = { filter: { beamTypes: beamTypes } };
const { lhcFills } = await new GetAllLhcFillsUseCase().execute(getAllLhcFillsDto)

expect(lhcFills).to.be.an('array').and.lengthOf(3)
Expand Down
11 changes: 5 additions & 6 deletions test/lib/usecases/run/GetAllRunsUseCase.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,23 +210,22 @@ module.exports = () => {
});

it('should successfully filter on beamTypes', async () => {
const singleBeamType = 'p-p';
const multipleBeamTypes = 'p-p,Pb-Pb';
const nonExistentBeamType = 'DOES-NOT-EXIST';
const singleBeamType = ['p-p'];
const multipleBeamTypes = ['p-p', 'Pb-Pb'];
const nonExistentBeamType = ['DOES-NOT-EXIST'];

getAllRunsDto.query = { filter: { beamTypes: singleBeamType }, page: { limit: 200 } };
{
const { runs } = await new GetAllRunsUseCase().execute(getAllRunsDto);
expect(runs).to.have.lengthOf.greaterThan(0);
expect(runs.every(({ lhcFill }) => lhcFill?.beamType === singleBeamType)).to.be.true;
expect(runs.every(({ lhcFill }) => singleBeamType.includes(lhcFill?.beamType))).to.be.true;
}

getAllRunsDto.query = { filter: { beamTypes: multipleBeamTypes }, page: { limit: 200 } };
{
const acceptedBeamTypes = multipleBeamTypes.split(',');
const { runs } = await new GetAllRunsUseCase().execute(getAllRunsDto);
expect(runs).to.have.lengthOf.greaterThan(0);
expect(runs.every(({ lhcFill }) => acceptedBeamTypes.includes(lhcFill?.beamType))).to.be.true;
expect(runs.every(({ lhcFill }) => multipleBeamTypes.includes(lhcFill?.beamType))).to.be.true;
}

getAllRunsDto.query = { filter: { beamTypes: nonExistentBeamType } };
Expand Down