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

Add syntax validation for payload formatters #3442

Closed
kschiffer opened this issue Nov 5, 2020 · 3 comments
Closed

Add syntax validation for payload formatters #3442

kschiffer opened this issue Nov 5, 2020 · 3 comments
Assignees
Labels
c/console This is related to the Console c/identity server This is related to the Identity Server duplicate This issue or pull request already exists technical debt Not necessarily broken, but could be done better/cleaner ui/cli This is related to ttn-lw-cli

Comments

@kschiffer
Copy link
Member

Summary

We currently allow any string value to be set as payload formatter. This makes it hard for users to find out about basic syntax issues in their payload formatter scripts.

Why do we need this?

  • Assisting users better when setting payload formatters
  • Avoid easily preventable setting of faulty payload formatters

What is already there? What do you see now?

In the console we have some basic syntax highlighting that notifies about issues in the script, however

  1. This mechanism only checks against a broad JavaScript syntax, while PFs use ES5 specifically
  2. Even when fixing this to use the correct syntax, CLI users will still not have any validation

What is missing? What do you want to see?

  1. The IS preventing to set PFs with syntax errors
  2. The IS responding with a helpful error message that includes the nature and stack trace of the error

Environment

v3.10.0

How do you propose to implement this?

Haven't worked on this but it's likely not a big issue to pipe the error output of the interpreter to the API response.

How do you propose to test this?

Unit tests.

Can you do this yourself and submit a Pull Request?

@johanstokking (please reassign as appropriate)

@kschiffer kschiffer added c/console This is related to the Console c/identity server This is related to the Identity Server ui/cli This is related to ttn-lw-cli technical debt Not necessarily broken, but could be done better/cleaner labels Nov 5, 2020
@kschiffer kschiffer added this to the Backlog milestone Nov 5, 2020
@johanstokking
Copy link
Member

We can probably catch a few things already when evaluating the script. However, we'd need to check all code execution paths to see if the script makes sense. This is already being enforced by the Device Repository.

In any case, catching low hanging fruit errors like entering Lua instead of JavaScript, or using ES6 language features, is easy to catch. Application Server can do that.

@trlafleur
Copy link

yes, this was a great feature in V2, made it easy to test JS prior to deploying...
It allowed sending a test string to the decoder and displaying its output...

@johanstokking
Copy link
Member

I see. I mark this as a duplicate of #2232 then. Please upvote there.

@johanstokking johanstokking added the duplicate This issue or pull request already exists label Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/console This is related to the Console c/identity server This is related to the Identity Server duplicate This issue or pull request already exists technical debt Not necessarily broken, but could be done better/cleaner ui/cli This is related to ttn-lw-cli
Projects
None yet
Development

No branches or pull requests

3 participants