elixir-cloud-aai / proTES

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

fix: infinite loop with jsonschema>3.2.0 #173

Open athith-g opened 3 months ago

athith-g commented 3 months ago

Describe the bug Starting proTES fails with jsonschema versions > 3.2.0. This is due to the tesNextTes schema in pro_tes/api/additional_logs.yaml referencing itself, causing an infinite loop during validation or reference resolution (I'm not sure which).

To Reproduce Steps to reproduce the behavior (in local python virtual environment):

  1. pip install -r requirements.txt
  2. pip install jsonschema==4.22.0
  3. cd pro_tes
  4. python3 app.py

Result Server gets stuck here: Screenshot 2024-05-17 at 10 31 47 AM

Expected behavior Server should continue with setup process.

Workarounds

  1. Use jsonschema version 3.2.0 (pip install jsonschema==3.2.0) OR
  2. Comment out forwarded_to property in tesNextTes OR
  3. Use oneOf:
        forwarded_to:
           oneOf:
               - $ref: '#/components/schemas/tesNextTes'

Possible solutions

  1. Specify jsonschema version in requirements.txt
  2. Alter additional_logs.yaml to avoid infinite loop
uniqueg commented 3 months ago

Nice catch, thanks a lot!

So we do need the forwarded_to property in order to keep track of the service that the TES requests ends up on eventually. And we also need to support multiple services in the chain, in case more than one gateway is involved.

But probably we could get away with a non-recursive solution, and luckily we can easily change that, given that it's not part of the TES specs but something we added ourselves. My suggestion would be to make tesNextTes an array of (a new model) tesForward with properties id, url and - to be on the safe side in case we cannot rely on ordering - a forward_time (use the same format as described in this comment. But remove the recursive forwarded_to reference.

This solution should work fine for use cases where forwarding is strictly linear. This assumption may not be warranted for all use cases, but as far as I know, we hadn't yet had a reason to forward an incoming task to multiple services (and anyway proTES doesn't currently support that).