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

Draft of new REST API using DRF #529

Closed
wants to merge 1 commit into from

Conversation

mAAdhaTTah
Copy link
Contributor

This pulls in DRF to configure our API. Pretty straightforward binding of a view
to a serializer & a model and making the data available. For this first pass,
we're using the model even though it's currently unstable. From a feature
standpoint, we get a lot for free from DRF with very little code, including
pagination. The list_links method loads all of the snapshots, which would
require pagination to be implemented manually on the entire list of snapshots,
which won't work well on large databases. Because archivebox is a CLI first
and a web application second, the way Exceptions are thrown and errors
logged doesn't always make those methods conducive to integrating w/ an API.

On the testing side, this shows up in how we're configuring things. The
setup_django function doesn't fully work when passing out_path;
Some variables in the Django settings aren't updated or configured correctly.
Instead, we use subprocess the same way the other tests do to start up the
server and hit it with requests.

Summary

This is obviously a work in progress but wanted to get some feedback on the
direction. It would be helpful if the API functions exposed by archivebox were
more decoupled from the CLI context specifically, but I think we're going to
want to bind the Models directly (at least for querying).

Related issues

#496

Changes these areas

  • Bugfixes
  • Feature behavior
  • Command line interface
  • Configuration options
  • Internal architecture
  • Snapshot data layout on disk

This pulls in DRF to configure our API. Pretty straightforward binding of a view
to a serializer & a model and making the data available. For this first pass,
we're using the model even though it's currently unstable. From a feature
standpoint, we get a lot for free from DRF with very little code, including
pagination. The `list_links` method loads all of the snapshots, which would
require pagination to be implemented manually on the entire list of snapshots,
which won't work well on large databases. Because archivebox is a CLI first
and a web application second, the way Exceptions are thrown and errors
logged doesn't always make those methods conducive to integrating w/ an API.

On the testing side, this shows up in how we're configuring things. The
`setup_django` function doesn't fully work when passing `out_path`;
Some variables in the Django settings aren't updated or configured correctly.
Instead, we use `subprocess` the same way the other tests do to start up the
server and hit it with `requests`.

# Summary

This is obviously a work in progress but wanted to get some feedback on the
direction. It would be helpful if the API functions exposed by archivebox were
more decoupled from the CLI context specifically, but I think we're going to
want to bind the Models directly (at least for querying).

# Related issues

ArchiveBox#496

# Changes these areas

- [ ] Bugfixes
- [X] Feature behavior
- [ ] Command line interface
- [ ] Configuration options
- [ ] Internal architecture
- [ ] Snapshot data layout on disk
@mAAdhaTTah mAAdhaTTah marked this pull request as draft November 8, 2020 21:19
@pirate
Copy link
Member

pirate commented Nov 10, 2020

This looks great! It's going in a good direction and the implementation looks concise and straightforward so far. Thanks for doing the test setup too.

@mAAdhaTTah mAAdhaTTah marked this pull request as ready for review November 14, 2020 21:13
@mAAdhaTTah mAAdhaTTah marked this pull request as draft November 14, 2020 21:13
@mAAdhaTTah
Copy link
Contributor Author

(Didn't mean to make ready.)

What's the semantic meaning of the added, updated, & bookmarked (which is derived from timestamp, which I believe is kind of like added but for Pocket, both API + export, it's the time the link was archived)? Is there a goal of removing one of these? Should these be customizable via the API? e.g. it kinda looks like the timestamp, and thus bookmarked can be set via the Pocket import; should the REST API enable that too?

@pirate pirate changed the title Draft of REST API Draft of new REST API using DRF Apr 6, 2021
@pirate
Copy link
Member

pirate commented Sep 4, 2023

Going to close this as stale. Thanks for all your work on this so far.

I'm planning to use https://django-ninja.rest-framework.com/ going forward for the REST API (instead of DRF).

@pirate pirate closed this Sep 4, 2023
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.

2 participants