-
-
Notifications
You must be signed in to change notification settings - Fork 320
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 function API #1022
Introduce function API #1022
Conversation
Also includes implementation for converting to SA filter spec.
Apparently dataclass defaults don't carry over from mixins.
I was wrong. Apparently I just got the order of mixin application wrong.
There's nothing faux about it.
Requesting re-review @kgodey @mathemancer. Changes implemented. See this comment for a list. @mathemancer I did not end up using your suggested setup. Current setup works ok and seems simple enough. I had fun exploring your idea though. Most changes are minimal and to the point. A noteworthy change is that I lowercased database type names in the table API. It seems that the SQLAlchemy method used to get those type names was uppercasing them. The rest of our code and at least some of the other SA methods return/expect database type names in lower case, so I forced it on the output of that method too. |
I may have jumped the gun. There are failing tests due to switching to lowercase for database type names on the table API. |
@dmos62 Why update the existing API, why not use the same SQLAlchemy method in the new API? Edited to add: Updating the exisitng API also makes this a breaking API change, see workflow here. |
This reverts commit 06cea56.
@kgodey I'm reverting my changes to the casing of database types. After investigating further, it seems that there's a fair amount of logic about the casing of those types, which I didn't notice earlier and I don't fully understand yet, and I'm not comfortable making any changes to it at the moment. I think that resolving this is outside the scope of this PR. I could uppercase the ids in the db_types endpoint as a partial workaround.
Because I'm working with database type Enums found under |
@dmos62 Can you open a new issue to track this? I agree it makes sense to not block this PR on it, but I don't think the Ideally, we would have a single source of truth for type IDs and use those everywhere. |
@kgodey agreed. I'll open a ticket for this tomorrow. |
Opened the ticket for inconsistent type name casing #1036 |
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.
@dmos62 Nice work, I had some more comments, mainly about the APIs. One of my previous comments is also unaddressed and I added a comment about that.
("date", "eq", "2000-01-01", 1), | ||
("array", "eq", "{0,0}", 1), | ||
# ne | ||
# ("varchar", "ne", "string42", 99), |
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.
@dmos62 This comment has not been addressed
mathesar/api/viewsets/db_types.py
Outdated
class DBTypeViewSet(viewsets.ViewSet): | ||
def list(self, request): | ||
try: | ||
db_name = request.query_params['db_name'] |
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.
We should be using IDs to refer to API resources, not names. Also, this shouldn't be a required parameter, we should be able to loop through all installed DBs if this is not provided. Filtering by DB ID should be handled similarly to other APIs, see mathesar/api/filters.py
.
If there's some reason to require a database, then it's better to put this under /api/v0/databases/<id>/types/
. We should not have multiple patterns to provide a database identifier in the URL.
All of this feedback also applies to the functions 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.
Fixed. Made new endpoints sub-resources of databases. So /api/v0/databases/<id>/db_types/
and /api/v0/databases/<id>/functions/
. When creating the db
and abstractions
namespaces, I'll split the DatabasesViewSet
into two view sets. Will have to think of a way to not retreat to calling one of them DbDatabasesViewSet
. I'll probably create Python namespaces: something like mathesar.api.db.viewsets.DatabasesViewSet
and mathesar.api.abstractions.viewsets.DatabasesViewSet
.
|
||
|
||
class DbFunctionSerializer(serializers.Serializer): | ||
id = serializers.CharField() |
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 looks like you've instead chosen to only make these available on a single database. In that case, they should not be a top level resource, please see my comment below. If we do choose to make them available on multiple databases, then you'll need to specify the database ID in the serializer.
@kgodey @mathemancer re-requesting review. Made requested changes. Namely, removed commented tests and made new endpoints' routing coherent with the rest. |
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.
Looks good to me, I'll defer to @mathemancer to complete review and merge.
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 noticed a problem with the function that checks whether a type is installed. To be honest, that logic is a bit all over the place both in the db/types/base.py
file as well as the db/types/operations/cast.py
file, and should probably be tidied up. I'm okay with deferring fixing the problem I noted for a future PR that also cleans up those functions generally. I'll approve, and leave it to you to decide whether to make the change now or later (or perhaps it's just not needed).
def get_available_known_db_types(engine): | ||
""" | ||
Returns database types that are both available on the database and known through our Enums | ||
above. | ||
""" |
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 doesn't check the DB, but rather gets the types which are both available in the engine mapping, as well as the Enums. Does it need to ensure the type is installed on the DB in order to fulfill its purpose? If not, the doc string should change. Otherwise, we should add a way to check that.
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 use available in this context to mean that it's installed on the DB. I made a mistake thinking that the engine mapping is a proxy for that.
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 was going by the docstring on db/types/base.py::get_available_types
:
def get_available_types(engine):
"""
Returns a dict where the keys are database type names defined on the database associated with
provided Engine, and the values are their SQLAlchemy classes.
"""
return engine.dialect.ischema_names
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.
The original docstring was also a mistake it seems.
@mathemancer I'll merge this PR in now, and I'll open a ticket to track the problem with checking whether a type is installed. |
If you don't mind, tag me (or ping me in matrix) when you do. We should make sure the ticket covers the whole problem, which has sprawled across multiple files at this point. |
@mathemancer sure, here's the ticket: #1050. |
This is a continuation of the Backend Filtering Refactor PR (#921).
This PR does not affect the current APIs. It only adds the endpoints and data structures related to the new function API, but does not replace the previous APIs. There will be another PR that will replace the current filtering API with a new filtering API that will use this function API.
Technical details
Should be considered a minor PR, since the functions API doesn't affect the rest of the code and its main "user" in the foreseeable future will be the to-be-implemented filtering API. In other words, this adds an internal API that will not yet be used on merge.
There are some minimal coverage tests for checking if
DbFunction
subclasses can be used for filtering. I expect to add tests as implementation breadth improves and as the upcoming filtering API is built.There are two significant improvements planned for the functions API (though not for this PR):
Adds the endpoints
/api/v0/functions
and/api/v0/db_types
.Here are their sample responses. Note that the hints currently applied are not accurate and are not meant to be at this stage. The uses of the hints system are somewhat unclear at this stage, since the frontend will not be using the functions API directly to build up the UI. The near-term future of the hints system depends on what the new filtering API will be like, which, as I said, is yet to be seen. I'd expect the principle of the hints system to be under review and not the details of their current use.
`/api/v0/functions` endpoint sample response
`/api/v0/db_types` endpoint sample response
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin