Refactor ApiHeaderValidationMiddleware: - #1114
Conversation
* Cleans up header validation for Content-Type and Accept * Corrects the logic so that JSON requests are validated correctly * Encapsulates checks in helper methods for improved readability and maintainability
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Hi there, were those changed made because of a real necessity or issues you had with the way it worked before? Your description is a little sparse on that. |
|
Hi there, i add more infos in the first comment. ;) |
|
Alright, sounds good. Could you please extend the tests with your specific use case? |
…ods and improve content negotiation logic; update tests for correct header handling (RFC9110, RFC8259)
|
After an additional research, i made a full rework based on RFC 9110 and RFC 8259. The tests are extended too. |
There was a problem hiding this comment.
Pull request overview
This PR refactors API header validation to better handle JSON media types with parameters and improve API content negotiation behavior.
Changes:
- Adds parsing for
Content-Typevalues such asapplication/json; charset=utf-8. - Introduces Accept-header negotiation with q-values and wildcard handling.
- Adds happy-path middleware tests for charset and q-value Accept headers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
app/Http/Middleware/ApiHeaderValidationMiddleware.php |
Refactors API header validation, content negotiation, and error response generation. |
tests/Middleware/ContentTypeHeaderValidationMiddlewareTest.php |
Adds new success cases for charset and Accept-header handling. |
…ponse error type codes in the header validation middleware test unit.
|
Most looks good, still unsure about the response format. Will have to think about that. |
|
I have taken another look at this and there are three changes I would like to comment on:
|
Hi, I made these changes because I'm having a problem with an Android app where a "charset" is also included in the Content-Type header.
For example: Content-Type "application/json; charset=utf-8".
Copilot also confirmed in a code review that the character set can be included here. Android also includes the "charset" by default.
The following changes was made: