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

Simplify checking for incorrect extractor and loader blocks #505

Merged
merged 5 commits into from
Jan 16, 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,21 +76,6 @@ describe('Validation of BlockDefinition', () => {
);
});

it('should diagnose error on block as output without having an output', async () => {
const text = readJvTestAsset(
'block-definition/invalid-output-block-as-input.jv',
);

await parseAndValidateBlock(text);

expect(validationAcceptorMock).toHaveBeenCalledTimes(1);
expect(validationAcceptorMock).toHaveBeenCalledWith(
'error',
'Blocks of type TestTableLoader do not have an output',
expect.any(Object),
);
});

it('should diagnose error on block as input for multiple pipes', async () => {
const text = readJvTestAsset(
'block-definition/invalid-block-as-multiple-pipe-inputs.jv',
Expand Down
31 changes: 10 additions & 21 deletions libs/language-server/src/lib/validation/checks/block-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,6 @@ function checkPipesOfBlock(
const blockType = new BlockTypeWrapper(block?.type);
const pipes = collectPipes(block, whatToCheck);

const isStartOrEnd =
(whatToCheck === 'input' && !blockType.hasInput()) ||
(whatToCheck === 'output' && !blockType.hasOutput());
if (isStartOrEnd) {
if (pipes.length > 0) {
for (const pipe of pipes) {
context.accept(
'error',
`Blocks of type ${blockType.type} do not have an ${whatToCheck}`,
whatToCheck === 'input'
? pipe.getToDiagnostic()
: pipe.getFromDiagnostic(),
);
}
}
return;
}

const hasMultipleInputPorts = pipes.length > 1 && whatToCheck === 'input';
if (hasMultipleInputPorts) {
for (const pipe of pipes) {
Expand All @@ -70,8 +52,6 @@ function checkPipesOfBlock(
return;
}

// above: there should be pipes (defined by blocktype)
// but there aren't any
if (pipes.length === 0) {
const isLastBlockOfCompositeBlocktype =
isCompositeBlocktypeDefinition(block.$container) &&
Expand All @@ -81,9 +61,18 @@ function checkPipesOfBlock(
isCompositeBlocktypeDefinition(block.$container) &&
block.$container.blocks.at(0)?.name === block.name;

const isExtractorBlock = !blockType.hasInput() && whatToCheck === 'input';
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming is somewhat confusing since it may be an ExtractorBlock even if we don't check for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be refactored in a future PR.

const isLoaderBlock = !blockType.hasOutput() && whatToCheck === 'output';

// exception: the first block in a composite block is connected to the input, not another block
// exception: The last block in a composite block is connected to the output, not another block
if (!isLastBlockOfCompositeBlocktype && !isFirstBlockOfCompositeBlocktype) {
// exception: the extractor / loader block in a pipeline
if (
!isLastBlockOfCompositeBlocktype &&
!isFirstBlockOfCompositeBlocktype &&
!isExtractorBlock &&
!isLoaderBlock
) {
context.accept(
'warning',
`A pipe should be connected to the ${whatToCheck} of this block`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,29 @@ describe('Validation of PipeDefinition', () => {
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
2,
'error',
`The output type "File" of TestFileExtractor is incompatible with the input type "Table" of TestTableLoader`,
'The output type "File" of block "TestExtractor" (of type "TestFileExtractor") is not compatible with the input type "Table" of block "TestLoader" (of type "TestTableLoader")',
expect.any(Object),
);
});

it('should diagnose error on connecting loader block to extractor block with a pipe', async () => {
const text = readJvTestAsset(
'pipe-definition/invalid-output-block-as-input.jv',
);

await parseAndValidatePipe(text);

expect(validationAcceptorMock).toHaveBeenCalledTimes(2);
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
1,
'error',
'Block "BlockTo" cannot be connected to other blocks. Its blocktype "TestFileLoader" has output type "None".',
expect.any(Object),
);
expect(validationAcceptorMock).toHaveBeenNthCalledWith(
2,
'error',
'Block "BlockFrom" cannot be connected to from other blocks. Its blocktype "TestFileExtractor" has input type "None".',
expect.any(Object),
);
});
Expand Down
23 changes: 17 additions & 6 deletions libs/language-server/src/lib/validation/checks/pipe-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,23 @@ function checkBlockCompatibility(
const fromBlockType = new BlockTypeWrapper(fromBlockTypeDefinition);
const toBlockType = new BlockTypeWrapper(toBlockTypeDefinition);

if (fromBlockType.hasOutput() && toBlockType.hasInput()) {
if (!fromBlockType.canBeConnectedTo(toBlockType)) {
const errorMessage = `The output type "${fromBlockType.outputType}" of ${fromBlockType.type} is incompatible with the input type "${toBlockType.inputType}" of ${toBlockType.type}`;
context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic());
context.accept('error', errorMessage, pipeWrapper.getToDiagnostic());
}
const isFromBlockLoader = !fromBlockType.hasOutput();
const isToBlockExtractor = !toBlockType.hasInput();

if (isFromBlockLoader) {
const errorMessage = `Block "${pipeWrapper.from?.name}" cannot be connected to other blocks. Its blocktype "${fromBlockType.astNode.name}" has output type "${fromBlockType.outputType}".`;
context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic());
}

if (isToBlockExtractor) {
const errorMessage = `Block "${pipeWrapper.to?.name}" cannot be connected to from other blocks. Its blocktype "${toBlockType.astNode.name}" has input type "${toBlockType.inputType}".`;
context.accept('error', errorMessage, pipeWrapper.getToDiagnostic());
}

if (!fromBlockType.canBeConnectedTo(toBlockType)) {
const errorMessage = `The output type "${fromBlockType.outputType}" of block "${pipeWrapper.from?.name}" (of type "${fromBlockType.astNode.name}") is not compatible with the input type "${toBlockType.inputType}" of block "${pipeWrapper.to?.name}" (of type "${toBlockType.astNode.name}")`;
context.accept('error', errorMessage, pipeWrapper.getFromDiagnostic());
context.accept('error', errorMessage, pipeWrapper.getToDiagnostic());
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,12 @@
// SPDX-License-Identifier: AGPL-3.0-only

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

block BlockFrom oftype TestFileExtractor {
}

BlockFrom -> BlockTo;

BlockTo -> BlockFrom;
}

Expand All @@ -19,7 +17,7 @@ builtin blocktype TestFileExtractor {
output outPort oftype File;
}

builtin blocktype TestTableLoader {
builtin blocktype TestFileLoader {
input inPort oftype File;
output outPort oftype None;
}
Loading