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

Allow RegExp values for jsonSchema.pattern field in property definition #4228

Open
10 tasks
bajtos opened this issue Nov 29, 2019 · 6 comments
Open
10 tasks
Labels
developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue Hacktoberfest Tasks ready for new contributors to work on Repository Issues related to @loopback/repository package

Comments

@bajtos
Copy link
Member

bajtos commented Nov 29, 2019

When creating a model, I'd like to specify pattern-based validation for my phoneNum property:

  @property({
    type: 'string',
    jsonSchema: {
      pattern: /\d{3}-\d{3}-\d{4}/,
    },
  })
  phoneNum?: string;

At the moment, we require the pattern to be a string source:

pattern: '\\d{3}-\\d{3}-\\d{4}'

I find these strings problematic:

  • Editors/IDEs don't highlight RegExp keywords
  • It's easy to forget to escape \ characters. When they are not escaped, users end up with a different RegExp that expected - see Use \d in AJV pattern doesn't work #4218 for an example.

Workaround

Use RegExp.source property to convert from a RegExp instance to a source string.

  @property({
    type: 'string',
    jsonSchema: {
      pattern: /\d{3}-\d{3}-\d{4}/.source,
    },
  })
  phoneNum?: string;

Acceptance criteria

Models (the important and also easy part)

Allow LB4 app developers to use RegExp pattern values in their model definitions, scoped to jsonSchema only for now (i.e. to apply during REST API validation only, not as a database constraint). Example property definition:

@property({
  jsonSchema: {
    pattern: /\d{3}-\d{3}-\d{4}/,
  },
})
phoneNum?: string;

REST API layer (optional, could be difficult to implement)

Allow LB4 app developers to use RegExp values in OpenAPI spec metadata, e.g. provided via Controller decorators.

class MyController {
  greet(
    @param({
      name: 'date',
      in: 'path',
      schema: {
        type: 'string',
        pattern: /^\d{4}-\d{2}-\d{2}$/,
      }
    })
    today: string,
  ) {
    return `Good morning, today is ${date}`;
  } 
}

Start by writing a group of (acceptance-level) tests to verify handling of RegExp patterns by our REST API layer:

  • Define a controller with a method with a parameter using pattern-based validation and a RegExp pattern value.
  • Send a request with a valid value expecting 200 response.
  • Send a request with an invalid value expecting 4xx response.
  • Verify that the OpenAPI spec document returned by the app includes the pattern converted to string via .source property. Quoting from https://github.com/OAI/OpenAPI-Specification/blob/3.0.1/versions/3.0.1.md#properties: pattern (This string SHOULD be a valid regular expression, according to the ECMA 262 regular expression dialect)
  • Fix the implementation (and type definitions) as necessary to allow such test to pass.
  • I think we will need to update @param definitions to allow RegExp values for pattern fields.
  • Figure out how to pass RegExp patterns to AJV so that it can perform pattern-base validation. It's possible that AJV supports RegExp values out of the box, in which case no changes may be required

Out of scope

pattern as a top-level model property:

@property({
  pattern: /\d{3}-\d{3}-\d{4}/,
  ...
})

Quoting from #4228 (comment):

The catch is that once pattern is a regular property metadata, it must be enforced both at REST API level (@loopback/rest must translate that pattern to AJV JSON Schema field) and at data-access level (by loopback-datasource-juggler), so it's more work to make this happen 🤷 The upside is that with a new property-level metadata, connectors can translate the pattern constraint to a database constraint too (a PostgreSQL example). Related: Epic: Validation at Model/ORM level #1872.


🎆 Hacktoberfest 2020

Greetings 👋 to all Hacktoberfest 2020 participants!

Here are few tips 👀 to make your start easier:

  • Before you start working on this issue, please leave a comment to let others know.
  • Feel free to implement support for RegExp patterns in model property definitions only, it's ok to leave the rest for others to pick up.
  • If you are new to GitHub pull requests, then you can learn about the process in Submitting a pull request to LoopBack 4.
  • If this is your first contribution to LoopBack, then please take a look at our Developer guide
  • Feel free to ask for help in #loopback-contributors channel, you can join our Slack workspace here.

See also #6456.

@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature labels Nov 29, 2019
@derdeka
Copy link
Contributor

derdeka commented Dec 11, 2019

JsonSchema is specifying pattern as string. AJV is also expecting string as input for pattern.

AFAIK the basic idea of openapi.json is to transport this pattern to the client and use the same rules as client validation too.

I think there is no benefit of allowing Regex for pattern here and customize JsonSchema in a non-standard-way. Or did I missunderstood something?

@achrinza
Copy link
Member

achrinza commented Dec 15, 2019

@derdeka This does seem like a non-standard feature at first glance but I think it can be beneficial.

As per-#4218, regexp has certain flags that use the backslash which may interfere with JavaScript escape character.

This means that those flags would need to be escaped with a double-backslash. This makes the final, written regexp non-standard when copy-and-pasted to elsewhere.

So this feature may benefit by allowing regexp to be written in the standardised format without having to write JavaScript escape characters.

@bajtos
Copy link
Member Author

bajtos commented Jan 10, 2020

I think there is no benefit of allowing Regex for pattern here and customize JsonSchema in a non-standard-way. Or did I missunderstood something?

The idea is to allow developers to write the pattern as a JavaScript regular expression and let the framework convert the RegExp object into a string pattern before passing it to JsonSchema/OpenApi/AJV or any other place.

The benefits for user experience are described in the issue description.

Unless you prefer to write your regular expressions as '\\d{3}-\\d{3}-\\d{4}', rather than as /\d{3}-\d{3}-\d{4}/? Check also the differences in code highlighting:

{
stringPattern: '\\d{3}-\\d{3}-\\d{4}',
regexPattern: /\d{3}-\d{3}-\d{4}/,
}

@bajtos
Copy link
Member Author

bajtos commented Jan 10, 2020

Check also the differences in code highlighting:

Hmm, that did not work out as expected. Here is a screenshot from GitHub's preview - I don't know why the final comment is rendered differently.

Screen Shot 2020-01-10 at 16 52 53

@achrinza achrinza added the Repository Issues related to @loopback/repository package label Jan 12, 2020
@achrinza
Copy link
Member

achrinza commented May 29, 2020

I was thinking about this, and I'm retracting away from my previous stance.

From my understanding, jsonSchema is passed to AJV as-is. Since the users should be able to swap AJV for a different validator, we should not modify what gets passed into jsonSchema.

Instead, could I propose we make a root pattern attribute:

@property({
  pattern: /\d{3}-\d{3}-\d{4}/,
  ...
})

This would put the responsibility on LB4 to validate the request instead of AJV.

This would also be in-line with providing first-class support for enum.

A downside would be feature duplication:

// This creates a double-validation from AJV and LB4

@property({
  pattern: /\d{3}-\d{3}-\d{4}/,
  jsonSchema: {
    pattern: /\d{3}-\d{3}-\d{4}/,
  },
})

But since AJV should be swappable, this may be a good step to moving away from over-reliance on AJV.

@bajtos
Copy link
Member Author

bajtos commented Jun 19, 2020

From my understanding, jsonSchema is passed to AJV as-is. Since the users should be able to swap AJV for a different validator, we should not modify what gets passed into jsonSchema.

I see your point. Can we perhaps enhance our built-in AJV version to allow pattern constraints to be expressed as RegExp instances? (Assuming AJV does not already support that.)

Another problem is that RegExp instances are serialized as empty objects in JSON. If we don't modify jsonSchema fields then we will end up with invalid OpenAPI spec :(

$ node
> JSON.stringify({pattern: /^a/})
'{"pattern":{}}'

Personally, I think it's ok to normalize jsonSchema.pattern:

  • The object provided in jsonSchema is expected to be a valid JSON Schema
  • JSON Schema says that pattern should be a string.
  • To improve developer experience, we allow users to provide the value as a RegExp instance and convert it to string (the only valid value type allowed by JSON Schema).

Instead, could I propose we make a root pattern attribute:

@property({
  pattern: /\d{3}-\d{3}-\d{4}/,
  ...
})

I like that idea 👍

The catch is that once pattern is a regular property metadata, it must be enforced both at REST API level (@loopback/rest must translate that pattern to AJV JSON Schema field) and at data-access level (by loopback-datasource-juggler), so it's more work to make this happen 🤷 The upside is that with a new property-level metadata, connectors can translate the pattern constraint to a database constraint too (a PostgreSQL example). Related: Epic: Validation at Model/ORM level #1872.

@bajtos bajtos added the Hacktoberfest Tasks ready for new contributors to work on label Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue Hacktoberfest Tasks ready for new contributors to work on Repository Issues related to @loopback/repository package
Projects
None yet
Development

No branches or pull requests

3 participants