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

Ess swap 2418 #20

Closed
wants to merge 8 commits into from
Closed

Ess swap 2418 #20

wants to merge 8 commits into from

Conversation

nitrosx
Copy link
Member

@nitrosx nitrosx commented Jun 3, 2022

This pull request will integrates the new functionalities added at ESS.
I also renamed functions according to the CRUD paradigm and mongodb functionalities (Regarding upsert)
Functionalities are still accessible with the all name, for backward compatibility.

I also added few new tests.

@nitrosx nitrosx requested a review from dylanmcreynolds June 3, 2022 12:42
@dylanmcreynolds
Copy link
Collaborator

dylanmcreynolds commented Jun 3, 2022

Thanks for the PR. I agree that consistent naming is necessary. I am a little worried about changing them PR by PR given that people are using the package already. But if we have to, I guess it's fine.

Part of the problem here is that SciCat has several endpoints with several names for essentially the same thing. Take Dataset loading, for example. To create a new dataset, you can choose a variety of endpoints and methods:

  • Datasets/ POST
  • Datasets/ PUT
  • Datasets/replaceOrCreate/ POST
  • Datasets/upsertWithWhere POST

To replace an existing dataset:

  • Datasets/{id} POST
  • Datasets/{id} PUT
  • Datasets/replaceOrCreate/ POST
  • Datasets/upsertWithWherePOST

We have similar issues with finding datasets.

At the same time, we need to be aware of the changes in new backend, which does not currently support the upsert. The new version is less loopbacky and more RESTful.

I think I agree that CRUD naming is good, but it's not quite enough, not capturing search or upsert. How about the following methods for each data type, taking Datasets as an example:

  • get (gets a list of objects, with optional filter parameters, GET to /datasets)
  • find (gets a list of object performing a search with optional query parameters, GET to /datasets/fullquery)
  • get_one (gets a single dataset by id, GET to /datasets/{id}
  • create (POST to /datasets)
  • update (PUT to /datasets/{id})
  • delete (DELETE to /datasets/{id})
  • upsert (performs a search based on dataset id, and then calls either get or update)

If we decide on a consistent pattern that can be applied to both versions of the backend, then we can thing about refactoring the client into multiple modules, on per root data type (datasets, proposals, samples, etc)

What do you thing, @nitrosx ?

@nitrosx
Copy link
Member Author

nitrosx commented Jun 3, 2022

@dylanmcreynolds I like your idea with a slight change!!! Using Datasets as an example, here what I propose:

  • get_datasets (notice the plural, gets a list of objects, with optional filter parameters, GET to /datasets)
  • find_datasets (gets a list of object performing a search with optional query parameters, GET to /datasets/fullquery)
  • get_dataset (notice the singular, gets a single dataset by id, GET to /datasets/{id}
  • create_dataset (POST to /datasets)
  • update_dataset (PUT to /datasets/{id})
  • delete_dataset (DELETE to /datasets/{id})
  • upsert_dataset (performs a search based on dataset id, and then calls either get or update)

If you agree, I will make the changes to my PR.
One more thing, in my PR I kept the old naming for the time being, so people have time to update their code.
Once this PR is accepted, we can decide a deadline to remove the old naming.

@dylanmcreynolds
Copy link
Collaborator

dylanmcreynolds commented Jun 3, 2022

I am still also trying to keep in mind the idea that you put forth of merging methods into data types, so trying to take the noun out of the function name, how about:

  • get_many (gets a list of objects, with optional filter parameters, GET to /datasets)
  • find (gets a list of object performing a search with optional query parameters, GET to /datasets/fullquery)
  • get_one (notice the singular, gets a single dataset by id, GET to /datasets/{id}
  • create (POST to /datasets)
  • update (PUT to /datasets/{id})
  • delete (DELETE to /datasets/{id})
  • upsert (performs a search based on dataset id, and then calls either get or update)

@nitrosx
Copy link
Member Author

nitrosx commented Jun 6, 2022

@dylanmcreynolds Deal!!!
If we add methods to the data models, we will use the naming convention you proposed in the post above.
Meanwhile, I'm going to change my PR to the following naming (example is for datasets):

  • datasets_get_many (gets a list of objects, with optional filter parameters, GET to /datasets)
  • datasets_find (gets a list of object performing a search with optional query parameters, GET to /datasets/fullquery)
  • datasets_get_one (notice the singular, gets a single dataset by id, GET to /datasets/{id}
  • datasets_create (POST to /datasets)
  • datasets_update (PUT to /datasets/{id})
  • datasets_delete (DELETE to /datasets/{id})
  • datasets_upsert (performs a search based on dataset id, and then calls either get or update)

How do you feel about this? ...or would you like to use:

  • get_many_datasets
  • find_datasets
  • get_one_dataset
  • create_dataset
  • update_dataset
  • delete_dataset
  • upsert_datasets

This version confuses me with singular and plural.
Let me know that I change the PR accordingly.

@nitrosx
Copy link
Member Author

nitrosx commented Jun 16, 2022

@dylanmcreynolds Can you confirm that the following changes are inline with what you have in mind?

- replace_dataset (upload_dataset) -> datasets_replace ( [Raw|Derived]DataSets/replaceOrCreate POST), 
  obsolete, to be removed
- create_dataset (upload_new_dataset) -> datasets_create (Datasets POST)
- replace_raw_dataset (upload_raw_dataset) -> datasets_raw_replace (RawDataSets/replaceOrCreate POST), 
  obsolete, to be replaced by datasets_update (Datasets/{id} PUT)
- replace_derived_dataset (upload_derived_dataset) -> datasets_derived_replace (DerivedDatasets/replaceOrCreate POST)
  obsolete, to be replaced by datasets_update (Datasets/{id} PUT)
- upsert_raw_dataset -> datasets_raw_upsert (RawDatasets/upsertWithWhere POST) 
  obsolete, to be removed
- upsert_derived_dataset -> datasets_derived_upsert (DerivedDatasets/upsertWithWhere POST) 
  obsolete, to be removed
- create_dataset_datablock (upload_datablock) -> datasets_datablock_create ([Raw|Derived]Datasets/{id}/origdatablocks POST)
  obsolete, to be replaced by datasets_origdatablocks_create
- create_dataset_origdatablock (upload_dataset_origdatablock) -> datasets_origdatablock_create (Datasets/{id}/origdatablocks POST)
- create_dataset_attachment (upload_attachment) -> datasets_attachment_create ([Raw|Derived]Datasets/{id}/attachment POST) 
  To be modified to use endpoint Datasets/{id}/attachment 
- find_datasets_full_query (get_datasets_full_query) -> datasets/find (Datasets/fullquery GET)
- find_datasets (get_datasets) -> datasets_get_many (Datasets GET)     
- find_published_data (get_published_data) -> published_data_get_many (PublishedData GET)
- get_dataset_by_pid -> datasets_get_one (Datasets/{id} GET)
- get_instrument -> instruments_get_one (Instruments/[{id}|findOne] GET) 
  to be modified to use only Instrument/{id}. Maybe create instrument_get_one_by_name 
- get_sample -> samples_get_one (Samples/{id} GET)
- get_proposal -> proposals_get_one (Proposals/{id} GET)
- get_dataset_origdatablocks -> dataset_origdatablocks_get_one (Datasets/{id}/origdatablocks GET)
- delete_dataset -> datasets_delete (Datasets/{id} DELETE)

Once this PR is in, we can review and make the functionalities consistent

@nitrosx
Copy link
Member Author

nitrosx commented Jun 17, 2022

PR #24 superseeds this PR

@nitrosx nitrosx closed this Jun 17, 2022
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