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

Hopefully fix xdrip uploads #4106

Closed
wants to merge 2 commits into from
Closed

Hopefully fix xdrip uploads #4106

wants to merge 2 commits into from

Conversation

viq
Copy link
Contributor

@viq viq commented Nov 30, 2018

This fixes uploads from xdrip for me, though I do see following in xdrip:

REST-API problem: Unable to do REST API Download
java.lang.NullPointerException: Attempt to write to field 'java.lang.String com.eveningoutpost.dexdrip.Models.Treatments.enteredBy' on a null object reference
Attempt to write to field 'java.lang.String com.eveningoutpost.dexdrip.Models.Treaments.enteredBy' on a null object reference url: <my nightscout URL>

@parapenT1sta
Copy link

I merged and tested it... It's working! 👍

@viq
Copy link
Contributor Author

viq commented Nov 30, 2018

And xdrip stopped complaining about downloading... So it seems it's working fine for me.

@PieterGit
Copy link
Contributor

PieterGit commented Nov 30, 2018

@viq thanks for looking into this. I don't understand your fix at this moment.
If I look at the format in express package it says:

* In addition to canonicalized MIME types you may
 * also use extnames mapped to these types:
 *
 *    res.format({
 *      text: function(){
 *        res.send('hey');
 *      },
 *
 *      html: function(){
 *        res.send('<p>hey</p>');
 *      },
 *
 *      json: function(){
 *        res.send({ message: 'hey' });
 *      }
 *    });

so I don't understand why the old code didn't work. What content type is sent by xdrip?
Looking into this I think we should return 406 (Not Acceptable) instead of 405 (Method not allowed) for text/csv. Any http status guru around?

@PieterGit PieterGit requested a review from sulkaharo November 30, 2018 23:44
@PieterGit
Copy link
Contributor

PieterGit commented Nov 30, 2018

@jamorham
On #4104 (comment) you write that xdrip sends Content-Type: application/json; ; charset=utf-8. Are the two ; ; allowed in a content type? According to https://www.w3.org/Protocols/rfc1341/4_Content-Type.html I think it's not officially valid according to the Extended BNF notation. Nightscout should accept it according to https://en.wikipedia.org/wiki/Robustness_principle but seems a small bug with xdrip. Don't know if that is causing the issue, but just noticed and wonder why oref0 does not have these upload problems. oref0 uses curl -H "content-type: application/json", see https://github.com/openaps/oref0/search?q=application%2Fjson&unscoped_q=application%2Fjson

I hope to have a look at this PR later this weekend.

@viq
Copy link
Contributor Author

viq commented Dec 2, 2018 via email

@sulkaharo
Copy link
Member

What's the actual bug here? The code change touches content downloading, not uploading, while the ticket title says this is an upload bug

@viq
Copy link
Contributor Author

viq commented Dec 2, 2018 via email

@sulkaharo
Copy link
Member

I need to look into this further but I think what's actually wrong is this code:

    api.post('/entries/', ctx.authorization.isPermitted('api:entries:create'), function(req, res, next) {
      // setting this flag tells insert_entries to store the results
      req.persist_entries = true;
      next();
    }, insert_entries, format_entries);

Where the POST requests also pass the request to the format_entries function, which should AFAIK only be done for requests that load data.

@sulkaharo
Copy link
Member

Can people following this actually test #4114 and tell me if that works. I think I know what's up but need a confirmation

@viq
Copy link
Contributor Author

viq commented Dec 2, 2018 via email

@jamorham
Copy link

jamorham commented Dec 2, 2018

xDrip just sets the content type to application/json with okhttp and retrofit libraries. The format of the post request headers etc is handled by these libraries which are very widely used and so I would not expect them to be incompatible. I may even have messed up the cut and paste from the network dump. @PieterGit I think this is probably unrelated to the bug/problem in hand.

@sulkaharo
Copy link
Member

@jamorham I tracked down the issue and know what’s causing it but not exactly sure why - looks like something is not exactly right in the Node Express mime type based routing. Anyway, my other PR fixed this

@PieterGit
Copy link
Contributor

@viq @jamorham @parapenT1sta PR #4114 of @sulkaharo has been merged to dev and I think that will fix this issue in a better way than this PR. Can on of you confirm that this issue can be closed, because @sulkaharo fixed the xdrip upload?

@viq
Copy link
Contributor Author

viq commented Dec 4, 2018

@PieterGit I confirm that with a fix from #4114 xdrip uploads work for me, therefore I'm closing this request.

@viq viq closed this Dec 4, 2018
@viq viq deleted the patch-1 branch December 4, 2018 10:38
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.

5 participants