molgenis / molgenis-emx2

MOLGENIS EMX2, the latest version of the MOLGENIS data platform.
GNU Lesser General Public License v3.0
11 stars 16 forks source link

feat: simple 'rest' api #3919

Closed connoratrug closed 4 days ago

connoratrug commented 5 days ago

usage: GET [serverlocation]/[schema]/api/rest/[table]

What are the main changes you did:

how to test:

todo:

sonarcloud[bot] commented 5 days ago

Quality Gate Failed Quality Gate failed

Failed conditions
8.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

mswertz commented 4 days ago

I think you are creating here a shorthand for all graphql 'table' query fields am I right? Then it would be feature complete if one could also include the 'filter' object and some limit/offset , i.e., the root parameters standard to those fields.

And what if I would want to use graphql nested queries? I can imagine users would like that too.

Given these questions, my main question is what/who is the use case for this? What problem does it solve? Why would we want to add an additional endpoint we need to test and maintain? Because for most of the screens we need the metadata anyhow to build forms, define the filters shown, etc.

I think a better solution might be

connoratrug commented 4 days ago

I think you are creating here a shorthand for all graphql 'table' query fields am I right?

Yes

Then it would be feature complete if one could also include the 'filter' object and some limit/offset , i.e., the root parameters standard to those fields.

yes

And what if I would want to use graphql nested queries? I can imagine users would like that too.

use the gql api

Given these questions, my main question is what/who is the use case for this? What problem does it solve? Why would we want to add an additional endpoint we need to test and maintain? Because for most of the screens we need the metadata anyhow to build forms, define the filters shown, etc.

stop rebuilding table query client side, no need for verbose gql query

I think a better solution might be

  • autogeneration graphql fragments that would allow you to do a select * from table E.g. something like Pet{...PetAll} or even if we want to also enable query expansion Pet{...PetAllExanded}

hmm, this still not able to do a simple get table

  • and creation of graphl endpoint that actually combines data with the metadata with the data so it is optimized for form generation? E.g. FormFor(table=pet, id=...) returning something like {name:'birthdate',type:'date',value:'01/01/2024'},...}

yea but would suggest rest style, once you start adding field/construct non defined in the query its no longer really gql ,can do metadata map with field = key and use this for both table and row query

mswertz commented 4 days ago

I am not ready to maintain two overlapping APIs, but lets explore the idea of a simpler API a bit further and then also get some broader feedback once we get there.

For this to be a graphql replacement we would need generated docs (there is already some rudimentary swagger generator as you might know) and equivalent capabilities in terms of the queries.

P.S. i would like to postpone full work on this until component library is done, but I propose we continue designing it a b it more parallel. I have few more backend improvements in mind, and one that might greatly help this PR is my plan to also enable each row to have also an internal id. That would make ref updates doable without much metadata. (other improvements have to do with per table permission, per row permission and a better submission feature).

connoratrug commented 4 days ago

I am not ready to maintain two overlapping APIs, but lets explore the idea of a simpler API a bit further and then also get some broader feedback once we get there.

its not really a full overlapping api , just a small facade that we could mark as internal only / beta / unversioned

For this to be a graphql replacement we would need generated docs (there is already some rudimentary swagger generator as you might know) and equivalent capabilities in terms of the queries.

personally i think the swagger thing is nt needed , it not 'full-rest' more rest like

P.S. i would like to postpone full work on this until component library is done, but I propose we continue designing it a b it more parallel. I have few more backend improvements in mind, and one that might greatly help this PR is my plan to also enable each row to have also an internal id. That would make ref updates doable without much metadata. (other improvements have to do with per table permission, per row permission and a better submission feature).

understand but could speed up frontend build and run time perf with this facade, just a lot simple when viewed from the client