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

Fix tap for the chunked transfer encoding #251

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kimakan
Copy link
Contributor

@kimakan kimakan commented Oct 28, 2024

This PR fixes the issue Daiquri had with TOPCAT. It was impossible to use the TAP_UPLOAD feature because TOPCAT is sending chunked request which was not supported in Daiquiri.

For now I've limited the fix to the requests sent to /tap/... to avoid any surprises for other modules.

One important caveat of this fix - it only works with gunicorn because it provides access to the de-chunked body of the request. It doesn't work with the Django development server and I'm not sure about the compatibility with other servers.

Along with the changes in tap, I refactored some other code:

  • human2bytes returns only an integer (before it was float, Literal, None) or raises an exception
  • make_query_dict_upper_case uses a mutable QueryDict instead of copying the dict. There was an issue with copying if the request was bigger than 2.5 MB.

I'm not 100% happy with the solution since it requires gunicorn but for now I don't have anything better in mind. I'm open to suggestions.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11555587769

Details

  • 16 of 31 (51.61%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 64.007%

Changes Missing Coverage Covered Lines Changed/Added Lines %
daiquiri/tap/middleware.py 5 8 62.5%
daiquiri/core/utils.py 9 21 42.86%
Totals Coverage Status
Change from base Build 11249262646: 0.04%
Covered Lines: 4830
Relevant Lines: 7546

💛 - Coveralls

@jochenklar
Copy link
Member

jochenklar commented Oct 29, 2024

I did some tests and maybe this is not a daiquiri problem after all (although your middleware solution is not bad). I ran daiquiri behind an nginx reverse proxy using a minimal docker setup similar to https://github.com/jochenklar/rdmo-dev-containers/tree/main/nginx-proxy (without the alias) and:

server {
    listen 127.0.0.1:8080;

    server_name nginx.local;

    root /var/www/nginx.local;
    index index.html;

    client_max_body_size 100M;

    location /static {
        proxy_pass http://localhost:9001/;
    }

    location / {
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Proto $scheme;
        proxy_set_header X-Forwarded-Host $http_host;
        proxy_pass http://localhost:9000/;
    }
}

The django dev server is running on http://localhost:9000.

Then, I use TOPCAT with http://localhost:8080/tap and the query:

SELECT t.parallax p1, s.parallax p2 FROM daiquiri_data_obs.stars as s join tap_upload.t1 as t on t.id = s.id

(t1 is daiquiri_data_obs.stars queried before).

And in principle it works ... there is another bug in https://github.com/django-daiquiri/daiquiri/blob/dev-formatting/daiquiri/query/utils.py#L202 which need to be changed to:

for upload_column in job.metadata.get('upload_columns', []):

.

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.

3 participants