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.

Sign up for GitHub

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

Jump to bottom

Raw endpoints #646

Merged
merged 2 commits into from
Jan 5, 2022
Merged

Raw endpoints #646

merged 2 commits into from
Jan 5, 2022

Conversation

mgautierfr
Copy link
Member

@mgautierfr mgautierfr commented Dec 8, 2021 •
edited by kelson42
Loading

Fixes #631
Fixes #616

@codecov
Copy link

codecov bot commented Dec 8, 2021 •
edited
Loading

Codecov Report

Merging #646 (f93ea22) into master (78c1034) will decrease coverage by 0.02%.
The diff coverage is 68.00%.

❗ Current head f93ea22 differs from pull request most recent head dc15a9a. Consider uploading reports for the commit dc15a9a to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
- Coverage   66.34%   66.31%   -0.03%     
==========================================
  Files          54       54              
  Lines        3987     4029      +42     
  Branches     2032     2061      +29     
==========================================
+ Hits         2645     2672      +27     
- Misses       1341     1356      +15     
  Partials        1        1
Impacted Files Coverage Δ
src/server/internalServer.h 33.33% <ø> (ø)
src/server/response.h 100.00% <ø> (ø)
src/server/internalServer.cpp 82.64% <65.71%> (-1.26%) ⬇️
src/server/response.cpp 85.77% <73.33%> (-0.90%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78c1034...dc15a9a. Read the comment docs.

@mgautierfr mgautierfr marked this pull request as ready for review December 15, 2021 15:21
veloman-yunkan
veloman-yunkan requested changes Dec 16, 2021
View reviewed changes
test/server.cpp Outdated Show resolved Hide resolved
veloman-yunkan
veloman-yunkan reviewed Dec 23, 2021
View reviewed changes
src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/response.h Show resolved Hide resolved
@stale
Copy link

stale bot commented Jan 3, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jan 3, 2022
@stale stale bot removed the stale label Jan 4, 2022
veloman-yunkan
veloman-yunkan requested changes Jan 4, 2022
View reviewed changes
Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

A few last minor fixes. You are welcome to rebase&fixup for the last iteration.

src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/internalServer.cpp Show resolved Hide resolved
src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/response.cpp Outdated Show resolved Hide resolved
test/server.cpp Outdated Show resolved Hide resolved
@mgautierfr mgautierfr added this to the 10.0.0 milestone Jan 5, 2022
@kelson42
Copy link
Collaborator

kelson42 commented Jan 5, 2022

@mgautierfr If there is a ticket already in a milestone, can we agree to avoid to put the corresponding PR in the milestone as well? To me it conplexifies the overall understanding of what is in a milestone.

@mgautierfr
Extend ItemResponse and ContentResponse to return raw content.
160a74f
@mgautierfr
Add raw endpoint.
dc15a9a 
As the name suggests it, this endpoint is not smart :
It returns the content as it is and only if it is present
(no compatibility or whatever).

The only "smart" thing is to return a redirect if the entry is a redirect.
@mgautierfr mgautierfr merged commit c9eb319 into master Jan 5, 2022
@mgautierfr mgautierfr deleted the raw_endpoints branch January 5, 2022 14:31
rgaudin
rgaudin reviewed Jan 5, 2022
View reviewed changes
src/server/internalServer.cpp
// /raw/<bookName>/<kind>/foo
// ^^^^^ ^ ^
// 5 + 1 + 1 = 7
auto itemPath = request.get_url().substr(bookName.size()+kind.size()+7);
Copy link
Member

Choose a reason for hiding this comment

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

Haven't tested but is this --urlRootLocation safe ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mgautierfr This (late) comment desserves a reponse IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, request.get_url() returns the url relative to the root location (so without it).

@veloman-yunkan veloman-yunkan mentioned this pull request Jan 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgaudin rgaudin rgaudin left review comments

@kelson42 kelson42 kelson42 left review comments

@veloman-yunkan veloman-yunkan veloman-yunkan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.

Define some endpoints for metadata (and other) Expose counter on meta endpoint
4 participants
@mgautierfr @kelson42 @rgaudin @veloman-yunkan