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

Introduce ui and db API namespaces #1041

Merged
merged 16 commits into from
Feb 10, 2022
Merged

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Feb 3, 2022

Fixes #1029

Splits our API views into two namespaces:

  • /api/db/v0/*
    • hosts endpoints that describe and interact with Postgres objects relatively directly
  • /api/ui/v0/*
    • hosts endpoints that describe and interact with Mathesar abstractions that are useful for user interfaces, such as Mathesar types or filters

For example:

  • /api/db/v0/databases/1/functions/ describes the low-level functions API;
    • previously, /api/v0/databases/1/functions
  • /api/ui/v0/databases/1/types/ describes the high-level UI abstractions,
    • previously, /api/v0/databases/1/types

At the moment of writing, only the Mathesar types endpoint is on the ui namespace. The rest of the endpoints just get the db/ segment inserted in the middle. More ui endpoints will follow, e.g. the upcoming (revamped) filters API.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the master branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 3, 2022

@kgodey @pavish @seancolsen please review changes, particularly to API URL structure. I'm still making subtle changes, but I'd like to get a sanity-check review early.

@dmos62 dmos62 self-assigned this Feb 3, 2022
@dmos62 dmos62 added affects: architecture Improvements or additions to architecture work: backend Related to Python, Django, and simple SQL labels Feb 3, 2022
@dmos62 dmos62 added this to the [06] Working with Tables milestone Feb 3, 2022
@kgodey
Copy link
Contributor

kgodey commented Feb 3, 2022

@dmos62 Please associate this PR with the issue #1029. Also note that the issue renamed the "abstractions" namespace to "ui" for clarity and brevity.

@kgodey kgodey changed the base branch from master to function-api-denuked February 3, 2022 16:59
@kgodey kgodey self-assigned this Feb 3, 2022
@seancolsen
Copy link
Contributor

@dmos62 Having only read the PR description (and not spent the time to look at the diff), I can offer the following thoughts as a high-level sanity check:

  • Looks good!
  • I don't have an opinion on abst vs ui

@pavish
Copy link
Member

pavish commented Feb 4, 2022

@dmos62 Based on @kgodey's comment here I assumed we'd have the client-specific endpoints named as ui instead of abst.

I'm in favour of naming the client specific endpoints as ui since we may have more APIs in the future that are focused on things like frontend layout, which are not necessarily related to Mathesar abstractions

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 4, 2022

@pavish @seancolsen @kgodey I think ui is a better name. I hadn't noticed the proposal. I'll change abst to ui. Thanks for sanity check.

@dmos62 dmos62 changed the title Introduce abstractions and db API namespaces Introduce ui and db API namespaces Feb 4, 2022
@dmos62
Copy link
Contributor Author

dmos62 commented Feb 4, 2022

Changed title and description to say that we'll use ui + db instead of abst + db namespaces.

Base automatically changed from function-api-denuked to master February 7, 2022 14:31
@kgodey
Copy link
Contributor

kgodey commented Feb 7, 2022

@dmos62 Can you fix merge conflicts in this PR?

@pavish
Copy link
Member

pavish commented Feb 7, 2022

@dmos62 There are some frontend changes on master which are not present in this PR yet. We'll update the frontend urls once this PR is upto date with master.

@pavish
Copy link
Member

pavish commented Feb 8, 2022

@dmos62 @kgodey @seancolsen I've added a commit 5fbb064 for the frontend changes.

The commit is a quick workaround to move this PR fast, and I've created a follow-up issue #1054 to deal with frontend changes we'll need in the future.

@dmos62
Copy link
Contributor Author

dmos62 commented Feb 8, 2022

@pavish Excuse the premature ping. I see you've started working on the frontend changes 👍

@pavish
Copy link
Member

pavish commented Feb 8, 2022

@dmos62 The frontend changes are done as mentioned in the above comment. This PR can be marked ready, if this is the only change you've been waiting for.

@dmos62 dmos62 marked this pull request as ready for review February 8, 2022 13:52
@dmos62 dmos62 requested a review from a team February 8, 2022 13:52
@dmos62
Copy link
Contributor Author

dmos62 commented Feb 8, 2022

A change I've omitted to mention until now is that the file containing Django filters has been renamed to dj_filters.py: mathesar/api/filters.py → mathesar/api/dj_filters.py.

This was done, because we're introducing a lot of code, including namespaces, that are named "filters" and reference Mathesar filters. Having a mathesar/api/filters.py file is confusing, because that's a reference to a different filter (the Django filter). So, I've named the file dj_filter to distinguish it from, for example, the upcoming mathesar/api/serializers/filter.py or mathesar/filters/.

In retrospect, I should have done that with the PR that adds the new filtering API. In the interest of saving time, I'd rather not move this change to that PR.

@kgodey
Copy link
Contributor

kgodey commented Feb 8, 2022

In the interest of saving time, I'd rather not move this change to that PR.

That's fine.

Copy link
Contributor

@kgodey kgodey left a comment

Choose a reason for hiding this comment

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

Looks good, nice work @dmos62 and @pavish.

@@ -0,0 +1,59 @@
from mathesar.api.display_options import DISPLAY_OPTIONS_BY_TYPE_IDENTIFIER
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we name the test files similarly to the file they're testing, I'd consider renaming this to ui_types or putting this in tests/api/ui. Please make this change in a future PR, though, since I don't want to block merge.

@kgodey kgodey assigned seancolsen and unassigned kgodey and dmos62 Feb 8, 2022
@kgodey
Copy link
Contributor

kgodey commented Feb 8, 2022

@seancolsen This needs your approval as a frontend codeowner.

@kgodey kgodey enabled auto-merge February 8, 2022 23:39
@kgodey kgodey added pr-status: review A PR awaiting review and removed ready Ready for implementation labels Feb 8, 2022
@kgodey kgodey merged commit d010e11 into master Feb 10, 2022
@kgodey kgodey deleted the introduce-abstractions-api-namespace branch February 10, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: architecture Improvements or additions to architecture pr-status: review A PR awaiting review work: backend Related to Python, Django, and simple SQL work: frontend Related to frontend code in the mathesar_ui directory
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Split up existing APIs into "DB" and "UI" namespaces
6 participants