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

New interceptors and updates to existing ones #254

Merged
merged 13 commits into from
Sep 14, 2023
Merged

Conversation

agigao
Copy link
Contributor

@agigao agigao commented Jul 31, 2023

New interceptors and updates to existing ones

CORS headers interceptor

  1. The allowed origin is configurable from env variables and in state it looks for :cors-url under :deps key
  2. It also supports preflight requests

Error response
Error interceptor wrapped in bad request response instead of 200

Multipart/form-data support

  1. Tiny but subtle improvement of params interceptor
  2. Mock and Peridot library used for testing purposes as it allows us to simply mock multipart requests, and many others, but that's what we needed at the moment.

Coercion improvements

  1. The first change I propose here is avoiding nesting coercion result under param type (path, query, form) and just keeping it under params
  2. Validation error returns humanized details

kebab-camel request/request - experimental feature, up for discussion

If the back-end is Clojure and Front-end Javascript we encounter inconsistency of keys in the context of the language.
Hence the experimental solution that we can discuss:

  1. kebab-camel interceptor transforms each key upon request to kebab:
  • original request {:request {:params {:paramKey1 1 :paramKey2 2 :paramKey3 3}}}
  • request after interceptor {:request {:params {:param-key-1 1 :param-key-2 2 :param-key-3 3}}}
  1. The keys in the response body are also transformed from kebab to camel:
  • response before interceptor - {:response {:body {:param-key-1 1 :param-key-2 2 :param-key-3 3}}}
  • after {:response {:body {:paramKey1 1 :paramKey2 2 :paramKey3 3}}}

P.S. slf4j-simple is also added to deps.edn to finally get rid of warning at the beginning of http server.

Tester info

All tests are provided

Completion Checklist

  • Add description of the changes made here to the changelog file

Will be added after approval

  • Update the documentation as necessary

The same here

- The allowed origin is configurable from env variables and in state it looks for `:cors-url` under `:deps` key
- It also supports `preflight` requests
1. JWT has to skip check if request is preflight, so requests with `:options` method is skipped
2. Additional check presence of authorization token in the headers, as it was throwing exceptions due to usage of `str/split`
- Tiny but subtle improvement of params interceptor
- Mock and Peridot library used for testing purposes as it allows us to simply mock multipart requests, and many others, but that's what we needed at the moment.
1. The first change I propose here is avoiding nesting coercion result under param type (path, query, form) and just keeping it under params
2. Validation error returns humanized details
If the back-end is Clojure and Front-end Javascript we encounter inconsistency of keys in the context of the language.
Hence the experimental solution that we can discuss:

1. kebab-camel interceptor transforms each key upon request to kebab:
- original request `{:request {:params {:paramKey1 1 :paramKey2 2 :paramKey3 3}}}`
- request after interceptor `{:request {:params {:param-key-1 1 :param-key-2 2 :param-key-3 3}}}`

2. The keys in the response body are also transformed from kebab to camel:
- response before interceptor - `{:response {:body {:param-key-1 1 :param-key-2 2 :param-key-3 3}}}`
- after `{:response {:body {:paramKey1 1 :paramKey2 2 :paramKey3 3}}}`

P.S. `slf4j-simple` is also added to deps to finally get rid of warning at the beginning of http server.
@agigao agigao changed the title Interceptor updates New interceptors and updates to existing ones Jul 31, 2023
@agigao
Copy link
Contributor Author

agigao commented Jul 31, 2023

PR included wrapping error in ring/bad-request but it caused havoc in the tests.

@gmsvalente gmsvalente self-requested a review August 3, 2023 11:08
Copy link
Contributor

@gmsvalente gmsvalente left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@soulflyer soulflyer left a comment

Choose a reason for hiding this comment

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

Does this really need discussion? Seems like a useful thing to have around, and adding it won't alter anything unless the user chooses to use it.
If it really does need discussion then it should be a separate PR
(This is re: camel kebab commit)

@soulflyer
Copy link
Contributor

Should probably be 4 PRs, (This is only a comment, not a request for change)

Comment on lines 173 to 174
schemas (or (get-in state [:request-data :match :data method :parameters])
(get-in state [:request-data :match :data :parameters]))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd keep the merge as it was, to be able to define the path related parameters once, and method parameters separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

Comment on lines 176 to 178
(:path schemas) (valid? (:path schemas) path)
(:query schemas) (valid? (:query schemas) query)
(:form schemas) (valid? (:form schemas) form-params))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How the :query and :form parameters are processed if you defined :path parameters TOO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, the change undone.

@agigao
Copy link
Contributor Author

agigao commented Sep 13, 2023

Should probably be 4 PRs, (This is only a comment, not a request for change)

agreed.

@agigao agigao merged commit f3ad130 into main Sep 14, 2023
4 checks passed
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.

4 participants