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

Refactor block validation multiple pipe inputs #506

Merged
merged 2 commits into from
Jan 17, 2024
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -76,22 +76,6 @@ describe('Validation of BlockDefinition', () => {
);
});

it('should diagnose error on block as input for multiple pipes', async () => {
const text = readJvTestAsset(
'block-definition/invalid-block-as-multiple-pipe-inputs.jv',
);

await parseAndValidateBlock(text);

expect(validationAcceptorMock).toHaveBeenCalledTimes(2);
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
2,
'error',
'At most one pipe can be connected to the input of a TestTableLoader',
expect.any(Object),
);
});

it('should have no error on valid block definition', async () => {
const text = readJvTestAsset('block-definition/valid-block-definition.jv');

Expand Down
12 changes: 0 additions & 12 deletions libs/language-server/src/lib/validation/checks/block-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,6 @@ function checkPipesOfBlock(
const blockType = new BlockTypeWrapper(block?.type);
const pipes = collectPipes(block, whatToCheck);

const hasMultipleInputPorts = pipes.length > 1 && whatToCheck === 'input';
if (hasMultipleInputPorts) {
for (const pipe of pipes) {
context.accept(
'error',
`At most one pipe can be connected to the ${whatToCheck} of a ${blockType.type}`,
pipe.getToDiagnostic(),
);
}
return;
}

if (pipes.length === 0) {
const isLastBlockOfCompositeBlocktype =
isCompositeBlocktypeDefinition(block.$container) &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,27 @@ describe('Validation of CompositeBlocktypeDefinition', () => {

expect(validationAcceptorMock).toHaveBeenCalledTimes(0);
});

it('should diagnose error on block as input for multiple pipes', async () => {
const text = readJvTestAsset(
'composite-blocktype-definition/invalid-block-as-multiple-pipe-inputs.jv',
);

await parseAndValidateBlocktype(text);

// first 2 errors for multiple pipelines in test file
expect(validationAcceptorMock).toHaveBeenCalledTimes(4);
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
3,
'error',
'At most one pipe can be connected to the input of a block. Currently, the following 2 blocks are connected via pipes: "BlockFrom1", "BlockFrom2"',
expect.any(Object),
);
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
4,
'error',
'At most one pipe can be connected to the input of a block. Currently, the following 2 blocks are connected via pipes: "BlockFrom1", "BlockFrom2"',
expect.any(Object),
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { CompositeBlocktypeDefinition } from '../../ast/generated/ast';
import { ValidationContext } from '../validation-context';

import { validateBlocktypeDefinition } from './blocktype-definition';
import { checkMultipleBlockInputs } from './pipeline-definition';

export function validateCompositeBlockTypeDefinition(
blockType: CompositeBlocktypeDefinition,
Expand All @@ -16,6 +17,8 @@ export function validateCompositeBlockTypeDefinition(
validateBlocktypeDefinition(blockType, validationContext, evaluationContext);
checkHasPipeline(blockType, validationContext);
checkExactlyOnePipeline(blockType, validationContext);

checkMultipleBlockInputs(blockType, validationContext);
}

function checkHasPipeline(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,20 @@ describe('Validation of PipelineDefinition', () => {

expect(validationAcceptorMock).toHaveBeenCalledTimes(0);
});

it('should diagnose error on block as input for multiple pipes', async () => {
const text = readJvTestAsset(
'pipeline-definition/invalid-block-as-multiple-pipe-inputs.jv',
);

await parseAndValidatePipeline(text);

expect(validationAcceptorMock).toHaveBeenCalledTimes(2);
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
2,
'error',
'At most one pipe can be connected to the input of a block. Currently, the following 2 blocks are connected via pipes: "BlockFrom1", "BlockFrom2"',
expect.any(Object),
);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@
//
// SPDX-License-Identifier: AGPL-3.0-only

import { PipelineWrapper } from '../../ast';
import { PipelineDefinition } from '../../ast/generated/ast';
import { PipeWrapper, PipelineWrapper } from '../../ast';
import {
BlockDefinition,
CompositeBlocktypeDefinition,
PipelineDefinition,
} from '../../ast/generated/ast';
import { ValidationContext } from '../validation-context';
import { checkUniqueNames } from '../validation-util';

Expand All @@ -16,6 +20,8 @@ export function validatePipelineDefinition(
checkUniqueNames(pipeline.transforms, context);
checkUniqueNames(pipeline.valuetypes, context);
checkUniqueNames(pipeline.constraints, context);

checkMultipleBlockInputs(pipeline, context);
}

function checkStartingBlocks(
Expand All @@ -39,3 +45,77 @@ function checkStartingBlocks(
);
}
}

export function checkMultipleBlockInputs(
pipeline: PipelineDefinition | CompositeBlocktypeDefinition,
context: ValidationContext,
): void {
if (!PipelineWrapper.canBeWrapped(pipeline)) {
return;
}
const pipelineWrapper = new PipelineWrapper(pipeline);

const startingBlocks = pipelineWrapper.getStartingBlocks();
let alreadyMarkedPipes: PipeWrapper[] = [];
for (const startingBlock of startingBlocks) {
alreadyMarkedPipes = doCheckMultipleBlockInputs(
pipelineWrapper,
startingBlock,
alreadyMarkedPipes,
context,
);
}
}

/**
* Inner method to check recursively whether blocks in a pipeline have multiple inputs
* @param pipelineWrapper The wrapping pipeline
* @param block The current block
* @param alreadyMarkedPipes List of already visited pipes to avoid duplicate errors
* @param context The validation context
* @returns the updated @alreadyMarkedPipes with all marked pipes
*/
function doCheckMultipleBlockInputs(
pipelineWrapper: PipelineWrapper<
PipelineDefinition | CompositeBlocktypeDefinition
>,
block: BlockDefinition,
alreadyMarkedPipes: Array<PipeWrapper>,
context: ValidationContext,
): Array<PipeWrapper> {
const pipesFromParents = pipelineWrapper.getIngoingPipes(block);
if (pipesFromParents.length > 1) {
const parentBlockNames = pipesFromParents.map(
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
(pipe) => '"' + pipe.from?.name + '"',
);
for (const pipe of pipesFromParents) {
const wasAlreadyMarked = alreadyMarkedPipes.some((x) => pipe.equals(x));
if (wasAlreadyMarked) {
continue;
}

context.accept(
'error',
`At most one pipe can be connected to the input of a block. Currently, the following ${
pipesFromParents.length
} blocks are connected via pipes: ${parentBlockNames.join(', ')}`,
pipe.getToDiagnostic(),
);

alreadyMarkedPipes.push(pipe);
}
}

const children = pipelineWrapper.getChildBlocks(block);
for (const child of children) {
alreadyMarkedPipes = doCheckMultipleBlockInputs(
pipelineWrapper,
child,
alreadyMarkedPipes,
context,
);
}

return alreadyMarkedPipes;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// SPDX-FileCopyrightText: 2023 Friedrich-Alexander-Universitat Erlangen-Nurnberg
//
// SPDX-License-Identifier: AGPL-3.0-only

composite blocktype TestBlockType {
input inPort oftype None;
output outPort oftype None;

block BlockTo oftype TestTableLoader { }

block BlockFrom1 oftype TestFileExtractor { }

block BlockFrom2 oftype TestFileExtractor { }

inPort -> BlockFrom1 -> BlockTo -> outPort;
inPort -> BlockFrom2 -> BlockTo -> outPort;
}

builtin blocktype TestFileExtractor {
input inPort oftype None;
output outPort oftype Table;
}

builtin blocktype TestTableLoader {
input inPort oftype Table;
output outPort oftype None;
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,19 @@
composite blocktype TestBlock {

input inputName oftype None;
output outputName oftype Sheet;
output outputName oftype TextFile;

block FileExtractor oftype HttpExtractor { url: 'url'; }
block FileTextInterpreter oftype TextFileInterpreter {}

block FileCSVInterpreter oftype CSVInterpreter {
}
block FileTextInterpreter1 oftype TextFileInterpreter {}
block FileTextInterpreter2 oftype TextFileInterpreter {}

inputName
->FileExtractor
->FileTextInterpreter
->FileCSVInterpreter
->FileTextInterpreter1
->outputName;

inputName
->FileExtractor
->FileTextInterpreter
->FileCSVInterpreter
->FileTextInterpreter2
->outputName;
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@
// SPDX-License-Identifier: AGPL-3.0-only

pipeline Pipeline {
block BlockTo oftype TestTableLoader {
}
block BlockTo oftype TestTableLoader { }

block BlockFrom oftype TestFileExtractor {
}
block BlockFrom1 oftype TestFileExtractor { }

BlockFrom -> BlockTo;
block BlockFrom2 oftype TestFileExtractor { }

BlockFrom -> BlockTo;
BlockFrom1 -> BlockTo;

BlockFrom2 -> BlockTo;
}

builtin blocktype TestFileExtractor {
Expand Down
Loading