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

Add a simple REST API for fetching a file's ID #72

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Dec 6, 2023

Addresses #71

Adds a simple REST API for fetching a files ID or path

I'm using a single endpoint, /api/fileid and query parameters to identify the file

GET /api/fileid/id?path=...
GET /api/fileid/path?id=...

both return the same JSON response for a file

{
    "id": ...
    "path": ...
}

Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e668814) 85.49% compared to head (bb048b5) 88.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #72      +/-   ##
==========================================
+ Coverage   85.49%   88.38%   +2.88%     
==========================================
  Files           5        6       +1     
  Lines         531      568      +37     
  Branches       68       70       +2     
==========================================
+ Hits          454      502      +48     
+ Misses         55       44      -11     
  Partials       22       22              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 7, 2023

The test failures appear unrelated—something with readthedocs is failing.

Copy link
Collaborator

@dlqqq dlqqq left a comment

Choose a reason for hiding this comment

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

@Zsailer Thank you for contributing this (and especially for including unit tests)! Left a few comments below.

The CI failure is unrelated. I'll fix it in a separate PR and you can rebase on main after that PR is merged.

Do you need this change included in a release after this PR is merged?

jupyter_server_fileid/extension.py Show resolved Hide resolved
jupyter_server_fileid/handler.py Outdated Show resolved Hide resolved
jupyter_server_fileid/handler.py Outdated Show resolved Hide resolved
@dlqqq
Copy link
Collaborator

dlqqq commented Dec 8, 2023

The RTD failure is fixed in #73, which is merged. You should rebase on main and force push to fix your branch's CI.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 8, 2023

Thank you for the review, @dlqqq! I appreciate your time here!

I have a couple of follow-up questions before I push changes. Let me know what you think!

Do you need this change included in a release after this PR is merged?

Yeah, if you don't mind. I'm happy to handle the release too. It looks like y'all have Jupyter releaser set-up, so I should be able to cut a release.

@dlqqq
Copy link
Collaborator

dlqqq commented Dec 8, 2023

@Zsailer Thanks. I've resolved all of the feedback above, except my suggestion for splitting the endpoint in two.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 8, 2023

Cool cool, thanks David! I've split the endpoints into two. Let me know what you think.

I made a minor choice to raise a 404 if the entry isn't found in the sqlite3 DB. get_id returns None in this case (instead of raising an error), but I think the client to the server would want to see an error raised, right?

await jp_fetch("api/fileid/id", params={"path": "test"})

assert err.value.code == 404
assert err.value.message.startswith("The ID for file")
Copy link
Member Author

Choose a reason for hiding this comment

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

Though I don't love doing this, checking the message might be necessary to avoid false positives in this test.

If we changed the endpoint in the future and forgot to update the unit test, this unit test would otherwise pass when it should fail.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 12, 2023

Hey @dlqqq, gentle ping here for final review and merge. I can cut the release after that.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 18, 2023

I'm going to proceed on merging this, since all review has been addressed. Thanks for your reviews @dlqqq!

@Zsailer Zsailer merged commit a5e65c9 into jupyter-server:main Dec 18, 2023
22 checks passed
@Zsailer
Copy link
Member Author

Zsailer commented Dec 19, 2023

Released: https://github.com/jupyter-server/jupyter_server_fileid/releases/tag/v0.9.1

(I meant to release as a minor bump, i.e. 0.10.0. Somehow I missed the automatic bumping by releaser to a patch release. Apologies for not catching this; there were a few steps to get Jupyter releaser working so I missed this. I think it's okay to leave this as a patch release—instead of yanking and re-releasing—since the changes are additive and minimal enough.)

@dlqqq
Copy link
Collaborator

dlqqq commented Dec 19, 2023

@Zsailer Oh, really sorry about this. I was on vacation last week and forgot to update my GitHub icon to reflect that. Thanks for cutting the release independently. I agree that it's fine to keep this as v0.9.1.

@Zsailer
Copy link
Member Author

Zsailer commented Dec 19, 2023

No apologies needed! I'm glad you got some time away.

I'm here to help maintain 😎

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.

None yet

2 participants