-
Notifications
You must be signed in to change notification settings - Fork 34
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
Media handler #10869 #10870
base: master
Are you sure you want to change the base?
Media handler #10869 #10870
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10870 +/- ##
=========================================
Coverage 84.60% 84.60%
+ Complexity 20020 20016 -4
=========================================
Files 2636 2635 -1
Lines 69568 69554 -14
Branches 5616 5610 -6
=========================================
- Hits 58856 58847 -9
- Misses 7996 7997 +1
+ Partials 2716 2710 -6 ☔ View full report in Codecov by Sentry. |
return; | ||
} | ||
|
||
if ( !( webRequest.getRawPath().startsWith( "/site/" ) || webRequest.getRawPath().startsWith( "/admin/site/" ) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code doesn't belong here . It should have been caught earlier that url doesn't match a supported one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | ||
throw createNotFoundException(); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this time web request must be already portal request. Creating new instance to then throw it away is a waste
fd2dab8
to
47ecf86
Compare
throws Exception | ||
{ | ||
final Matcher matcher = PATTERN.matcher( Objects.requireNonNullElse( webRequest.getEndpointPath(), webRequest.getRawPath() ) ); | ||
final PortalRequest portalRequest = | ||
webRequest instanceof PortalRequest ? (PortalRequest) webRequest : new PortalRequest( webRequest ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is safe to say that for non-portal requests we fail it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it also apply to /api
? I am not sure
} | ||
|
||
if ( !IS_GET_HEAD_OPTIONS_METHOD.test( webRequest ) ) | ||
if ( !IS_ALLOWED_METHOD.test( portalRequest ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular benefit to define lambda as constant here? I don't see a reuse
{ | ||
return HandlerHelper.handleDefaultOptions( ALLOWED_METHODS ); | ||
} | ||
|
||
if ( !( portalRequest.isSiteBase() || portalRequest.getRawPath().startsWith( "/api/" ) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any benefit to double chech isSiteBase here and later?
|
||
final PortalRequest portalRequest = createPortalRequest( webRequest, repositoryId, branch ); | ||
portalRequest.setRepositoryId( repositoryId ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit shaky. Overriding request repo/branch fools downstream logic what was actually requested.
Maybe better to read all needed parameters from request and provide them as arguments to downstream methods instead of draging the entire request
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was done for a /api/...
request
e10f911
to
6afa583
Compare
No description provided.