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

first draft of v2 list API #234

Merged
merged 57 commits into from
Jun 2, 2023
Merged

first draft of v2 list API #234

merged 57 commits into from
Jun 2, 2023

Conversation

stschiff
Copy link
Member

@stschiff stschiff commented Feb 20, 2023

This is a first draft at the new Server List API. It will define four new APIs:

  • /v2/packages -> Lists Package Info and replaces the previous /packages_all
  • /v2/groups -> Lists Group Information and replaces the previous /groups_all
  • /v2/individuals -> Lists Individual information, including extra-columns, replaces previous /individuals_all
  • /v2/janno -> Lists the full Janno-Data, replaces previous /janno_all

Note that /zip_file/:package_name will remain as is.

All /v2/... APIs will support the official query-string syntax for URIs. For example /v2/packages?client_version=1.1.8.5 will be possible, and the client_version parameter is in fact one of the parameters that will be used in all of these V2-APIs.

All V2-APIs will return a data type that contains messages from the server to the client.

Another extra-parameters that will be supported is /v2/individuals?extra_columns=Country,AgeBCAD,... which will make it possible to query for additional Janno-information for all individuals without actually downloading the entire Janno. I think this will speed up significantly the communication time with the server.

We could even add a searchString URI parameter to be able to filter return data for specific search terms, but that's an extra feature likely outside of this PR.

@stschiff
Copy link
Member Author

stschiff commented Apr 25, 2023

Alright, so this PR is now at a stage where I need some feedback on how to continue. Here is what has been done so far:

  • A new set of APIs has been defined, the old ones are gone.
  • trident list now uses the new APIs
  • trident fetch uses the new APIs

Tested so far with running a server on localhost, which can for example be started with the following command from within the poseidon-hs source directory:

stack run poseidon-http-server -- -d test/testDat/testPackages -z ~/Desktop/zipTest

and can then be queried for example with

stack run trident -- fetch --remoteURL http://localhost:3000 -d ~/Desktop/testOut --downloadAll

or

stack run trident -- list --remote --remoteURL http://localhost:3000 --packages

What does not yet work are the tests for fetch and list, because there is currently no public server running the new APIs. I guess there are three options for the tests:

  • Option 1: Disable the server-communication tests temporarily. I was gonna do that, but pedantic compilation then requires a few more code lines to be commented out/removed, and so I stopped for now.
  • Option 2: Adapt the tests to run a local server. This would be ideal at some point, but it requires some learning curve for how to run a concurrent job within a test-suite. Not rocket science, but not implemented at the moment
  • Option 3: Install the new server on a public host, such as our new virtual server

Option 2 and 3 require some time, so I wanted to get feedback, also considering that Pull Requests should not become too large to review.

I also would like to mention the following decisions/issues that we might have to discuss:

  1. The server now provides all packages with all versions in its base directory
  2. /individuals, /groups and /packages now return entities from all packages in all versions.
  3. Whenever packages are printed via list, whether locally or remotely, they are appended with the version if possible, as in Schiffels_2016-1.1.0.
  4. fetch now also appends the out-directory for packages with the version, and --downloadAll downloads and saves all versions of all packages. This could be changed of course, but that's what I currently have.
  5. list --packages, both when run locally and remotely, shows a lot more information than previously, including package and Poseidon versions of a package, the description and date of last change.
  6. Various functions and types that are required for ServerClient communication are currently scattered over Package.hs, SecondaryTypes.hs and Utils.hs. This will have to be refactored eventually, perhaps by creating a new module ServerClientCommunication.hs, and eventually moving all content from SecondaryTypes there or into Utils.hs. To be discussed.

@stschiff
Copy link
Member Author

OK, I have now made the server-tests work offline, by starting a server in the background. Works. I think this is ready to review now, which isn't terribly easy, unfortunately, because I had to factor out the Server code to a new module to be runnable from the test suite.

I think from a perspective of a user, the main outstanding feature is now a necessary extension of our fetch- and forge-?) DSL to specify a version of a package. See also my notes above.

@stschiff stschiff marked this pull request as ready for review May 10, 2023 15:48
@stschiff
Copy link
Member Author

The new server (from this branch) is now running on https://server.poseidon-adna.org. I have updated the default URL in trident to this new address.

@stschiff
Copy link
Member Author

stschiff commented Jun 1, 2023

OK, live tests work. I think this is ready. Still can't figure out what's going on with stylish-haskell though

@stschiff
Copy link
Member Author

stschiff commented Jun 1, 2023

OK, stylish-haskell was fixed by adapting the version on my computer.

@nevrome
Copy link
Member

nevrome commented Jun 2, 2023

With fetch the server greets twice. Polite, but redundant 🙂 :

...
[Info]    Downloading individual list from remote
[Info]    Message from the Server: Greetings from the Poseidon Server, version 1.2.0.0
[Info]    Downloading package list from remote
[Info]    Message from the Server: Greetings from the Poseidon Server, version 1.2.0.0
...

@nevrome
Copy link
Member

nevrome commented Jun 2, 2023

Ok - so there is one inconsistency we may want to reconsider: list --packages returns the package version in an extra column, whereas --groups and --individuals appends the version to the package name. I find the extra column cleaner. Could we homogenize this?

@nevrome
Copy link
Member

nevrome commented Jun 2, 2023

fetch doesn't feel fully consistent yet.

Imagine I have a package A-1.0.0 in a local directory myLovelyPackageA and the Poseidon server already has A-2.0.0.
If I then run fetch -d . -f "*A*" nothing will be downloaded:

[Info]    Comparing local and remote package A-2.0.0
[Info]    A                     local 1.0.0 < remote 2.0.0 (overwrite with --upgrade)

If I want the latest version, then my only option is to append --upgrade to the command. If I do this, the latest version of the package will be downloaded, but not by overwriting the previous package in myLovelyPackageA, but by creating a new directory A-2.0.0.

This feels a bit misleading. I think previously we just assumed that the directory names would be same and fetch could just safely overwrite with --upgrade. That was probably too naive already (because the new package version could have less files). Now the directory names don't fit any more and the term --upgrade sounds misleading.

I'm not sure what the best interface would be. What is the default usecase? Having the latest package version, right? But we can not simply delete a user's packages... Here's an idea: If fetch encounters an old version of a requested package, it reports this overlap and does nothing else. If the user adds --overwrite, then the old version is explicitly deleted and the new version is downloaded. If the user adds --addinstead, then the new version gets downloaded, but without removing the old one.--overwriteand--addthus replace --upgrade`. I think this would work well - except when the package is not just a set of files in one directory. Then it's hard (yet not impossible...) to delete the package properly. But this problem already existed for the old implementation.

@stschiff
Copy link
Member Author

stschiff commented Jun 2, 2023

With fetch the server greets twice. Polite, but redundant 🙂 :

...
[Info]    Downloading individual list from remote
[Info]    Message from the Server: Greetings from the Poseidon Server, version 1.2.0.0
[Info]    Downloading package list from remote
[Info]    Message from the Server: Greetings from the Poseidon Server, version 1.2.0.0
...

Fixed in latest commit

@stschiff
Copy link
Member Author

stschiff commented Jun 2, 2023

fetch doesn't feel fully consistent yet.

Imagine I have a package A-1.0.0 in a local directory myLovelyPackageA and the Poseidon server already has A-2.0.0. If I then run fetch -d . -f "*A*" nothing will be downloaded:

[Info]    Comparing local and remote package A-2.0.0
[Info]    A                     local 1.0.0 < remote 2.0.0 (overwrite with --upgrade)

If I want the latest version, then my only option is to append --upgrade to the command. If I do this, the latest version of the package will be downloaded, but not by overwriting the previous package in myLovelyPackageA, but by creating a new directory A-2.0.0.

This feels a bit misleading. I think previously we just assumed that the directory names would be same and fetch could just safely overwrite with --upgrade. That was probably too naive already (because the new package version could have less files). Now the directory names don't fit any more and the term --upgrade sounds misleading.

I'm not sure what the best interface would be. What is the default usecase? Having the latest package version, right? But we can not simply delete a user's packages... Here's an idea: If fetch encounters an old version of a requested package, it reports this overlap and does nothing else. If the user adds --overwrite, then the old version is explicitly deleted and the new version is downloaded. If the user adds --addinstead, then the new version gets downloaded, but without removing the old one.--overwriteand--addthus replace --upgrade`. I think this would work well - except when the package is not just a set of files in one directory. Then it's hard (yet not impossible...) to delete the package properly. But this problem already existed for the old implementation.

Fixed in latest commit, as discussed.

@nevrome nevrome merged commit 526f609 into master Jun 2, 2023
@nevrome nevrome deleted the server_api_v2 branch June 2, 2023 14:12
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