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

feat: OData implicit globals detection #510

Merged
merged 7 commits into from
Jan 31, 2025
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
24 changes: 24 additions & 0 deletions src/linter/binding/BindingLinter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,4 +152,28 @@ export default class BindingLinter {
this.#context.addLintingMessage(this.#resourcePath, MESSAGE.PARSING_ERROR, {message}, position);
}
}

#isExpressionBinding(bindingDefinition: string): boolean {
return /^\{:?=/.test(bindingDefinition) && bindingDefinition.endsWith("}");
}

lintPropertyExpression(
Copy link
Contributor Author

@d3xter666 d3xter666 Jan 31, 2025

Choose a reason for hiding this comment

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

It's not possible to use the BindingParser directly as it strips the information of the globals. However, in the runtime code a warning for those gets fired: https://github.com/SAP/ui5-linter/blob/main/src/linter/binding/lib/ExpressionParser.js#L125

The code in the ExpressionParser does not significantly differs from the current implementation. It just checks whether those globals are present in the expression

bindingDefinition: string, requireDeclarations: RequireDeclaration[], position: PositionInfo) {
if (!this.#isExpressionBinding(bindingDefinition)) {
return;
}
const allFunctionsModule = "sap/ui/model/odata/ODataExpressionAddons";
const varModuleMap = {
"odata.compare": "sap/ui/model/odata/v4/ODataUtils",
"odata.fillUriTemplate": "sap/ui/thirdparty/URITemplate",
"odata.uriEncode": "sap/ui/model/odata/ODataUtils",
};

for (const [key, value] of Object.entries(varModuleMap)) {
if (bindingDefinition.includes(`${key}(`) &&
!requireDeclarations.some((decl) => decl.moduleName === value || value === allFunctionsModule)) {
this.#context.addLintingMessage(this.#resourcePath, MESSAGE.NO_ODATA_GLOBALS, {} as never, position);
}
}
}
}
11 changes: 11 additions & 0 deletions src/linter/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export enum MESSAGE {
NO_EXPORTED_VALUES_BY_LIB,
NO_GLOBALS,
NO_ICON_POOL_RENDERER,
NO_ODATA_GLOBALS,
NOT_STATIC_CONTROL_RENDERER,
PARSING_ERROR,
PARTIALLY_DEPRECATED_CORE_ROUTER,
Expand Down Expand Up @@ -614,4 +615,14 @@ export const MESSAGE_INFO = {
`Please import the module itself directly instead of accessing it via the library module.`,
},

[MESSAGE.NO_ODATA_GLOBALS]: {
severity: LintMessageSeverity.Error,
ruleId: RULES["no-implicit-globals"],

message: () => "OData built-in global symbols must not be used implicitly",
details: () =>
"Import the 'sap/ui/model/odata/ODataExpressionAddons' module instead. " +
"See {@link topic:28fcd55b04654977b63dacbee0552712 Best Practices for Developers}",
},

} as const;
4 changes: 4 additions & 0 deletions src/linter/xmlTemplate/Parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,10 @@ export default class Parser {
line: prop.start.line + 1, // Add one to align with IDEs
column: prop.start.column + 1,
});
this.#bindingLinter.lintPropertyExpression(prop.value, this.#requireDeclarations, {
line: prop.start.line + 1, // Add one to align with IDEs
column: prop.start.column + 1,
});
} else if (this.#apiExtract.isAggregation(`${namespace}.${moduleName}`, prop.name)) {
this.#bindingLinter.lintAggregationBinding(prop.value, this.#requireDeclarations, {
line: prop.start.line + 1, // Add one to align with IDEs
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
<mvc:View xmlns="sap.m" xmlns:mvc="sap.ui.core.mvc" xmlns:core="sap.ui.core">
<mvc:View xmlns="sap.m" xmlns:mvc="sap.ui.core.mvc" xmlns:core="sap.ui.core" core:require="{
bar: 'sap/ui/model/odata/ODataUtils'
}">

<Input
value="{
path: 'amount',
Expand All @@ -12,4 +15,19 @@
core:require="{
Integer: 'sap/ui/model/type/Integer'
}"/>

<Text text="{= odata.compare(%{myvalue1},%{myvalue2})}" core:require="{
foo: 'sap/ui/model/odata/v4/ODataUtils'
}" />

<!-- "sap/ui/model/odata/ODataExpressionAddons" contains all of the odata globals -->
<Text text="{= odata.compare(${myvalue1},${path: 'myvalue1', model: 'myModel'})}" core:require="{
foo: 'sap/ui/model/odata/ODataExpressionAddons'
}" />

<!-- Should not be reported. Module already required with the previous binding -->
<Text text="{= odata.compare(%{myvalue1},%{myvalue2})}" />

<!-- Should not be reported. Module already required in the root -->
<Text text="{= odata.uriEncode(%{myvalue1})}" />
</mvc:View>
3 changes: 3 additions & 0 deletions test/fixtures/transpiler/xml/XMLViewBindings.view.xml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
xmlns:core="sap.ui.core"
controllerName="com.myapp.controller.Main"
>

<Text text="{= odata.compare(%{myvalue1},%{myvalue2})}" />

<!-- text: Deprecated reference of global formatter and binding event handler function -->
<!-- tooltip: Aggregation with a 0..1 aggregation and "string" alt type can be used like a property -->
<!-- customData: Aggregation with factory and nested filter -->
Expand Down
30 changes: 22 additions & 8 deletions test/lib/linter/xmlTemplate/snapshots/transpiler.ts.md
Original file line number Diff line number Diff line change
Expand Up @@ -1335,7 +1335,12 @@ Generated by [AVA](https://avajs.dev).
import ObjectStatus2 from "sap/m/ObjectStatus";␊
import ObjectStatus3 from "sap/m/ObjectStatus";␊
import ObjectStatus4 from "sap/m/ObjectStatus";␊
import Text from "sap/m/Text";␊
import View from "sap/ui/core/mvc/View";␊
const oText = new Text({␊
text: "{= odata.compare(%{myvalue1},%{myvalue2})}",␊
});␊
const oObjectStatus = new ObjectStatus({␊
text: "{\\n\\t\\t\\tpath: 'invoice>Status',\\n\\t\\t\\tformatter: 'ui5.walkthrough.model.formatter.statusText',\\n\\t\\t\\tevents: {\\n\\t\\t\\t\\tdataRequested: 'global.onMyDataRequested'\\n\\t\\t\\t}\\n\\t\\t}",␊
tooltip: "{\\n\\t\\t\\tpath: 'invoice>StatusDetails',\\n\\t\\t\\tformatter: 'globalTooltipFormatter'\\n\\t\\t}",␊
Expand All @@ -1359,6 +1364,7 @@ Generated by [AVA](https://avajs.dev).
export default const oView = new View({␊
controllerName: "com.myapp.controller.Main",␊
content: [␊
oText,␊
oObjectStatus,␊
oObjectStatus2,␊
oObjectStatus3,␊
Expand All @@ -1372,7 +1378,7 @@ Generated by [AVA](https://avajs.dev).

{
file: 'XMLViewBindings.view.js',
mappings: 'AAsDE,4CAKE;AALF,iCAKE;AALF,uCAKE;AALF,wDAKE;AAlDH,8CA6BE;AAGF,+CAKE;AAEF,+CAEE;AAGF,+CA6BE;AAlFH,wCAIC;sBAKA,kBA6BE;IA7BY,6LAMX;IAEF,qGAGE;IAEF,wVAcE;IACF,kCAAgC;;;uBAIjC,mBAKE;IAJD,6EAGE;;;uBAGH,mBAEE;IADD,kGAAwG;;;uBAIzG,mBA6BE;IAtBD,+LAME;IACF,iVAcE;;;6BAjFJ,UAIC;IADA,4CAA0C;IAM1C,OA6BE',
mappings: 'AAyDE,4CAKE;AALF,iCAKE;AALF,uCAKE;AALF,wDAKE;AAlDH,8CA6BE;AAGF,+CAKE;AAEF,+CAEE;AAGF,+CA6BE;AA/EF,8BAA0D;AAN3D,wCAIC;cAEA,UAA0D;IAApD,mDAAiD;;;sBAMvD,kBA6BE;IA7BY,6LAMX;IAEF,qGAGE;IAEF,wVAcE;IACF,kCAAgC;;;uBAIjC,mBAKE;IAJD,6EAGE;;;uBAGH,mBAEE;IADD,kGAAwG;;;uBAIzG,mBA6BE;IAtBD,+LAME;IACF,iVAcE;;;6BApFJ,UAIC;IADA,4CAA0C;IAG1C,OAA0D',
names: [],
sources: [
'XMLViewBindings.view.xml',
Expand All @@ -1383,57 +1389,65 @@ Generated by [AVA](https://avajs.dev).
> messages

[
{
column: 8,
line: 7,
message: 'OData built-in global symbols must not be used implicitly',
messageDetails: 'Import the \'sap/ui/model/odata/ODataExpressionAddons\' module instead. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-implicit-globals',
severity: 2,
},
{
column: 16,
line: 10,
line: 13,
message: 'Access of global variable \'global\' (global.onMyDataRequested)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
severity: 2,
},
{
column: 16,
line: 10,
line: 13,
message: 'Access of global variable \'ui5\' (ui5.walkthrough.model.formatter.statusText)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
severity: 2,
},
{
column: 3,
line: 18,
line: 21,
message: 'Access of global variable \'globalTooltipFormatter\' (globalTooltipFormatter)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
severity: 2,
},
{
column: 3,
line: 23,
line: 26,
message: 'Access of global variable \'global\' (global.factoryFunction)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
severity: 2,
},
{
column: 3,
line: 23,
line: 26,
message: 'Access of global variable \'global\' (global.testMethod)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
severity: 2,
},
{
column: 3,
line: 23,
line: 26,
message: 'Access of global variable \'global\' (global.testMethod)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
severity: 2,
},
{
column: 3,
line: 43,
line: 46,
message: 'Access of global variable \'parseInt\' (parseInt)',
messageDetails: 'Do not use global variables to access UI5 modules or APIs. See Best Practices for Developers (https://ui5.sap.com/#/topic/28fcd55b04654977b63dacbee0552712)',
ruleId: 'no-globals',
Expand Down
Binary file modified test/lib/linter/xmlTemplate/snapshots/transpiler.ts.snap
Binary file not shown.