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

Change CORS config key from path-prefix to path-expr; method names also #1807

Merged
merged 5 commits into from
May 15, 2020
Merged

Change CORS config key from path-prefix to path-expr; method names also #1807

merged 5 commits into from
May 15, 2020

Conversation

tjquinno
Copy link
Member

Resolves #1767

The value is, in fact, used as a pattern (expression) and not a prefix.

Signed-off-by: tim.quinn@oracle.com tim.quinn@oracle.com

…ed as an expresion or pattern, not a prefix

Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
Signed-off-by: tim.quinn@oracle.com <tim.quinn@oracle.com>
@tjquinno tjquinno self-assigned this May 15, 2020
tomas-langer
tomas-langer previously approved these changes May 15, 2020
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM

@tomas-langer
Copy link
Member

A note for consistency - in Security, we have the following configuration:

security.web-server:
        paths:
            - path: "/noRoles"
              methods: ["get"]
            - path: "/user[/{*}]"
              methods: ["get"]
              roles-allowed: ["user"]

I use just path, even though it can be an expression (as in the example above).
I did not feel the expr was required here.
In routing, we call it pathPattern

This is adding a third option.
Please consider this - does it make sense to change all of them to expression, or use one of the existing terms?

Options:

  1. path
  2. path-pattern
  3. path-expr

I have approved this PR, as we may delay that decision for a future Helidon version...

@tjquinno
Copy link
Member Author

After a quick slack chat with Tomas, I am re-changing the config key name to path-pattern and associated method names, arguments, etc. to pathPattern to be consistent with similar usage elsewhere in WebServer. Update to the PR coming soon.

Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

I like this!

@tjquinno tjquinno merged commit c4d4703 into helidon-io:master May 15, 2020
@tjquinno tjquinno deleted the cors-path-expr branch May 15, 2020 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CORS mapped config should use path or path-expr not path-prefix
3 participants