elixir-cloud-aai / proTES

Proxy service for injecting middleware into GA4GH TES requests
Apache License 2.0
6 stars 6 forks source link

fix: add ignore_error executor field #176

Open athith-g opened 4 months ago

athith-g commented 4 months ago

Describe the bug TES v1.1 defines an ignore_error field as a property of an executor (defined here). Currently, the TesExecutor model does not have this attribute. As a result, the following error occurs when a response with the ignore_error is provided:

TypeError: Executor.__init__() got an unexpected keyword argument 'ignore_error'

Full traceback:

Screenshot 2024-06-07 at 3 34 46 PM

Executor causing error:

Screenshot 2024-06-07 at 3 35 34 PM

Software Using proTES with Funnel

uniqueg commented 3 months ago

Thanks for reporting. This issue is due to Funnel, but not proTES, having adopted TES v1.1.0. The library that throws this error is py-tes.

A few points:

  1. Upgrading py-tes to use the v1.1.0 branch should resolve this issue, but it's possible that we might run into other issues. For example, proTES may not forward the ignore_error value.
  2. Alternatively, downgrading Funnel to a version that adopts v1.0.0 should also fix the issue. However, as far as I know, the latest Funnel version addresses a number of issues and is not just an upgrade to TES v1.1.0, so this would rather be a short-term solution.
  3. The best way forward is probably to upgrade the proTES models to properly support v1.1.0, but making sure that it remains backwards-compatible.
  4. Apart from dealing with ignore_error in one way or another, py-tes should not actually throw this error. This is because additionalProperties is not set to false in the TES specs, meaning that returning additional specs is not violating the specs and should not throw an error. We should open an issue about that.

Do you want to take care of these issues, @athith-g? That is:

athith-g commented 3 months ago

@uniqueg For sure I'll update the proTES models and open up that issue.