SAP / open-ux-tools

Enable community collaboration to jointly promote and facilitate best in class tooling capabilities
Apache License 2.0
83 stars 34 forks source link

BUG - `@sap-ux/ui5-application-writer`: Invalid names allowed #1348

Open IainSAP opened 11 months ago

IainSAP commented 11 months ago

Description

Recent versions of ui5 tooling use spec version 3.1 which have specific rules for the project name, ( https://sap.github.io/ui5-tooling/stable/pages/Configuration/#name ) . Currently @sap-ux/ui5-application-writer writes specVersion: 3.1 however allows invalid names. This leads to a validation error when serving apps :

Error Message:
Invalid ui5.yaml configuration for project fpm_v4

Not a valid project name. It must consist of lowercase alphanumeric characters, dash, underscore, and period only. Additionally, it may contain an npm-style package scope. For details, see: https://sap.github.io/ui5-tooling/stable/pages/Configuration/#name

ui5.yaml:5

3: specVersion: "3.1"
4: metadata:
5:   name: testNameSpace.fpmv4

@sap-ux/ui5-application-writer has test cases using an invalid name: https://github.com/SAP/open-ux-tools/blob/main/packages/ui5-application-writer/test/index.test.ts#L31

https://sap.github.io/ui5-tooling/stable/pages/Configuration/#specification-versions

Steps to Reproduce

Run the tests for @sap-ux/ui5-application-writer. Notice the outputs are using an invalid name.

Expected results

Tests using an invalid name throw a validation error.

Actual results

Screenshots

If applicable, add screenshots to help explain the problem.

Version/Components/Environment

Add any other context about the problem here OS:

Root Cause Analysis

Problem

{describe the problem}

Fix

{describe the fix}

Why was it missed

{Some explanation why this issue might have been missed during normal development/testing cycle}

How can we avoid this

{if we don’t want to see this type of issues anymore what we should do to prevent}

tobiasqueck commented 11 months ago

@IainSAP reading this, I'd suggest to simply use the name we using the package.json also in the ui5.yaml. Then we should be good to go, correct?

IainSAP commented 11 months ago

@IainSAP reading this, I'd suggest to simply use the name we using the package.json also in the ui5.yaml. Then we should be good to go, correct?

The value used in the yaml is the app id provided through the API. Package name is a separate API property, so I think changing the behaviour would be unexpected. It looks like we are not validating the package name either but simply lowercasing it in the template.

There are specific rules for yaml since specVersion 3.1 https://sap.github.io/ui5-tooling/stable/pages/Configuration/#name so we should probably validate or actively modify the app id value to be compliant (since we are outputting specVersion 3.1)

tobiasqueck commented 11 months ago

The value used in the yaml is the app id provided through the API. Package name is a separate API property, so I think changing the behaviour would be unexpected. It looks like we are not validating the package name either but simply lowercasing it in the template.

With the change in specVersion 3.1 the app id in the id in the ui5.yaml are not compatible anymore, so we should not use the same in both places. With the 3.1 change it seems like the ui5.yaml is supposed to be in sync with the package.json, so what speaks against keeping them in sync?

Or, what would you suggest to do instead?

IainSAP commented 11 months ago

The value used in the yaml is the app id provided through the API. Package name is a separate API property, so I think changing the behaviour would be unexpected. It looks like we are not validating the package name either but simply lowercasing it in the template.

With the change in specVersion 3.1 the app id in the id in the ui5.yaml are not compatible anymore, so we should not use the same in both places. With the 3.1 change it seems like the ui5.yaml is supposed to be in sync with the package.json, so what speaks against keeping them in sync?

Or, what would you suggest to do instead?

Options:

1) We throw a validation error if the id does not comply with either: 1) Manifest sap.app id validation or 2) specVer 3.1 ui5.yaml name.
2) We add a new property specifically for the yaml name that is validated to comply with the spec version rules 3) ...as you mention perhaps now the ui5.yaml is supposed to be aligned with the package.json name. So, we validate the package name according to npm package name rules and also, in case there is a difference, (e.g. min name length > 3) the specVer 3.1 name rules.

I don't have a preference, other than the fix is the least unexpected change in behaviour.