microsoft / fhir-server

A service that implements the FHIR standard
MIT License
1.19k stars 512 forks source link

Pagination broken - Next URL more than 2048 characters #1567

Open mikemassa84 opened 3 years ago

mikemassa84 commented 3 years ago

Describe the bug The "next" url in the link section of a Bundle resource results in a 404 response. I believe this is related to the URL specified as "next" is longer than 2048 characters. In our case, it is 5,399 characters. We are using the Azure API for FHIR.

FHIR Version? R4

Data provider? CosmosDB

To Reproduce Steps to reproduce the behavior:

  1. Send a GET response to /Encounter (We have > 100,000 Encounters)
  2. Copy the link of the URL tagged with relation: next in the link object.
  3. Send a GET to the link copied from step 2.
  4. You should get a 404 response if the URL is too long

Expected behavior Next page of Encounter resources should display

Actual behavior A 404 response message is returned saying The resource you are looking for has been removed, had its name changed, or is temporarily unavailable.

Here is our initial call to /Encounter: image

Here is the response when calling the URL from the next link: image

feordin commented 3 years ago

@mikemassa84 Thanks for the bug report. Could you let me know the Azure API for FHIR account name? It will be helpful in trying to debug this.

mikemassa84 commented 3 years ago

@mikemassa84 Thanks for the bug report. Could you let me know the Azure API for FHIR account name? It will be helpful in trying to debug this.

@feordin Name is nma1-s2-fhir Let me know if you need anything else.

feordin commented 3 years ago

@mikemassa84 I have tried to repro your issue, but have not been able to see the same error which you have shown. We have our service configured to handle Urls of up to 8kb, and Cosmos DB should be generating continuation tokens of up to 4kb. We then encode those continuation tokens in base64, which would match up with the length you see of 5399. When I attempt to query your instance with a url of approximately 5400 bytes in length, I receive an operation outcome with Authentication Failed error, which is what I expect to happen.

Is it possible you have a proxy server running? or other routing mechanism which might apply the 2k limit to the Url length?

Are you able to open an azure support ticket? It might be easier to continue debugging the issue with a more direct method of communication.

mikemassa84 commented 3 years ago

@feordin We do have a proxy in front of our fhir api. It is running as an Azure function and simply passes the request through as is. I'll try stepping through that code and see if the function is truncating the continuation token. If that's not the case, I'll have an Azure support ticket opened. My team also has a standing meeting with Microsoft on Fridays, so maybe we can pull you in that way as well?

mikemassa84 commented 3 years ago

@feordin I ran our fhir proxy function locally and it worked without issue. The failure is occurring when the proxy is running deployed to Azure.

feordin commented 3 years ago

I think it would be a great topic to bring up in your standing meeting on Friday. I saw this documentation (https://docs.microsoft.com/en-us/azure/azure-functions/functions-bindings-http-webhook-trigger?tabs=csharp) related to HTTP triggers for Azure functions which seem to indicate that the limit for a function url length is 4kb. It also seems to indicate that it may be configurable in the web.config file.

mikemassa84 commented 3 years ago

@feordin You can probably close this as the issue does seem related to the Azure functions being limited by maxQueryStringLength="4096" being set. Not sure if we are able to update that, but I'll reach out to support for it.

I imagine the FHIRProxy MS wrote would encounter the same issue? https://github.com/microsoft/health-architectures/tree/master/FHIR/FHIRProxy

mikemassa84 commented 3 years ago

@mikemassa84 I have tried to repro your issue, but have not been able to see the same error which you have shown. We have our service configured to handle Urls of up to 8kb, and Cosmos DB should be generating continuation tokens of up to 4kb. We then encode those continuation tokens in base64, which would match up with the length you see of 5399. When I attempt to query your instance with a url of approximately 5400 bytes in length, I receive an operation outcome with Authentication Failed error, which is what I expect to happen.

Do you think it would make sense to ensure the base64 encoded token does not exceed 4kb, since that is what is sent back by calling systems?

feordin commented 3 years ago

@mikemassa84 I have tried to repro your issue, but have not been able to see the same error which you have shown. We have our service configured to handle Urls of up to 8kb, and Cosmos DB should be generating continuation tokens of up to 4kb. We then encode those continuation tokens in base64, which would match up with the length you see of 5399. When I attempt to query your instance with a url of approximately 5400 bytes in length, I receive an operation outcome with Authentication Failed error, which is what I expect to happen.

Do you think it would make sense to ensure the base64 encoded token does not exceed 4kb, since that is what is sent back by calling systems?

@mikemassa84 Apologies for the delay in the response. I wanted to check in with the broader team to confirm making a change. Yes, we agree that making the continuation token smaller is a good idea, and it is on the backlog to be done. I am afraid I don't have an ETA on when it will happen.

I am curious if you were able to change the maxQueryStringLength of the azure function and mitigatge the issue for now?

mikemassa84 commented 3 years ago

@feordin Unfortunately, I could not find a way to change the maxQueryStringLength setting. I tried using ISS Manager extension for App Service to change the setting of the underlying IIS, but it didn't seem to make any difference. Either the extension doesn't work, I didn't use it properly, or the extension isn't relevant to function apps.

I was able to get in touch with some of the folks who work on the MS FHIR Proxy, so hopefully I can come up with something with them. Worst case scenario for me is I use my proxy for post/put/patch/delete operations and go directly to the fhir api for read operations.

CaitlinV39 commented 3 years ago

AB#78736

CaitlinV39 commented 3 years ago

We reduced the size of the token to 3K so this should resolve.

mikemassa84 commented 3 years ago

We reduced the size of the token to 3K so this should resolve.

This is still broken if you search by multiple parameters. Here's an example from our server:

/Patient?name=Smith&gender=female

This generated a ct with the following: W3sidG9rZW4iOiIrUklEOn44d0JSQUtzNzJndERxQThBQUFBQUFBPT0jUlQ6MSNUUkM6MTAjSVNWOjIjSUVPOjY1NTY3I1FDRjo3I0ZQUDpBZ2crQUFBQUFBQUFBRXdEQUFBQU1BQUFQZ0FBQUFBQUFBQUNBRU9vUHdBQUFBQUFBQUFFQUNXSm9wTkJBQUFBQUFBQUFBSUF3NlZIQUFBQUFBQUFBQUlBaXFGSUFBQUFBQUFBQUFJQXBJTkpBQUFBQUFBQUFBSUE1NU5OQUFBQUFBQUFBQUlBTkxKUUFBQUFBQUFBQUFJQU5LdFdBQUFBQUFBQUFBSUFWcmxYQUFBQUFBQUFBQUlBYllOWkFBQUFBQUFBQUFJQUtLUmFBQUFBQUFBQUFBUUEvYko1aEZzQUFBQUFBQUFBQWdDSGcxMEFBQUFBQUFBQUFnQXhtR0VBQUFBQUFBQUFBZ0E0cDJNQUFBQUFBQUFBQkFDR2tNaU5aQUFBQUFBQUFBQUNBTSs2WlFBQUFBQUFBQUFFQUtDTFc1Rm1BQUFBQUFBQUFBSUF4NUpuQUFBQUFBQUFBQVFBUmJ4ZmdXZ0FBQUFBQUFBQUFnQ1RzV2tBQUFBQUFBQUFBZ0JHa1cwQUFBQUFBQUFBQmdCempjU1E4NDl1QUFBQUFBQUFBQUlBc3FKd0FBQUFBQUFBQUFZQSs0TnZnY3V5Y1FBQUFBQUFBQUFDQU5lV2NnQUFBQUFBQUFBSUFLZVBySnVEaDJhRWN3QUFBQUFBQUFBQ0FQS2FkQUFBQUFBQUFBQUNBSG03ZFFBQUFBQUFBQUFFQUdXSDZwWjJBQUFBQUFBQUFBSUFYN0YzQUFBQUFBQUFBQUlBODZSNUFBQUFBQUFBQUFRQTBwSHZnM29BQUFBQUFBQUFDQUNjaEZPWmJvSHBnbnNBQUFBQUFBQUFBZ0NjaTN3QUFBQUFBQUFBQWdDMW5IMEFBQUFBQUFBQUJBQjRnZGlEZ2dBQUFBQUFBQUFDQUdXM2d3QUFBQUFBQUFBSUFCQ0NJbzh6a3NXVWhBQUFBQUFBQUFBSUFDS0s2SUllaEtPbWhRQUFBQUFBQUFBQ0FLbXZoZ0FBQUFBQUFBQUNBRW0waHdBQUFBQUFBQUFHQUFDZEFZdmNoWWdBQUFBQUFBQUFBZ0FabG9rQUFBQUFBQUFBQWdBL2w0MEJBQUFBQUFBQUJnQUZnY0NvK29xT0FRQUFBQUFBQUFJQUZJZVBBUUFBQUFBQUFBUUFESjNHaUpFQkFBQUFBQUFBQkFBQ2taeXRrZ0VBQUFBQUFBQUVBRGFOZEl5VEFRQUFBQUFBQUFZQVlZTWFpSldrbEFFQUFBQUFBQUFFQUdtQ0k3eVZBUUFBQUFBQUFBUUFFWXA1bHBZQkFBQUFBQUFBQkFCdHEwcVBtQUVBQUFBQUFBQUdBSk9GTnA5NW1wa0JBQUFBQUFBQUFnQUZzWm9CQUFBQUFBQUFDQUF6Z1Q2T0xwWGRocHNCQUFBQUFBQUFCQUJjaDF1Z25BRUFBQUFBQUFBQ0FPZWJuUUVBQUFBQUFBQUNBTUtEbndFQUFBQUFBQUFDQUlPNG9BRUFBQUFBQUFBQ0FLeUFvUUVBQUFBQUFBQUNBUCtIb2dFQUFBQUFBQUFFQUNPWHE1S2pBUUFBQUFBQUFBUUFKYW8va0tRQkFBQUFBQUFBQWdBaG1xY0JBQUFBQUFBQUJBQktrMENFcUFFQUFBQUFBQUFFQURlVStLZXBBUUFBQUFBQUFBUUE1NEdlajZvQkFBQUFBQUFBQkFDSXFZbURxd0VBQUFBQUFBQUNBQ1NPclFFQUFBQUFBQUFHQU0yZmlKbFZncTRCQUFBQUFBQUFBZ0JKb2JBQkFBQUFBQUFBQkFEMGpaR0NzUUVBQUFBQUFBQUVBSEtMZ0oreUFRQUFBQUFBQUFJQVVJS3pBUUFBQUFBQUFBWUFHWUQ4aTZhTHRBRUFBQUFBQUFBRUFMQ0JHWisxQVFBQUFBQUFBQUlBVW9xMkFRQUFBQUFBQUFRQWFKWWdtN2NCQUFBQUFBQUFBZ0FLdTdnQkFBQUFBQUFBQ0FBYWhjMkdrWk1BbDdrQkFBQUFBQUFBQkFCbXAvaUN1Z0VBQUFBQUFBQUVBTXEyMTRMQUFRQUFBQUFBQUFJQXU2bkNBUUFBQUFBQUFBSUFITHpEQVFBQUFBQUFBQUlBN2JuRUFRQUFBQUFBQUFRQXA2MlhpY1VCQUFBQUFBQUFDQURSanYrRU00RWNnY1lCQUFBQUFBQUFBZ0ROa3NnQkFBQUFBQUFBQkFCNGpEQ255UUVBQUFBQUFBQUNBTitNeWdFQUFBQUFBQUFHQUdhRVNZUmtzczBCQUFBQUFBQUFCQUFjajk2a3pnRUFBQUFBQUFBR0FNV0VacGpZb2M4QkFBQUFBQUFBQmdEMWh5bVk2WXJTQVFBQUFBQUFBQUlBRnJEVEFRQUFBQUFBQUFRQWlxT0ZsTlFCQUFBQUFBQUFCQUNhbEZpRDFRRUFBQUFBQUFBQ0FPMnIxd0VBQUFBQUFBQUNBQ09tMkFFQUFBQUFBQUFFQUppT3NwSGFBUUFBQUFBQUFBSUEwSkRiQVFBQUFBQUFBQVFBYTVuRWs5NEJBQUFBQUFBQUFnQUFrZDhCQUFBQUFBQUFBZ0QvaXVBQkFBQUFBQUFBQkFBZWorYWM0UUVBQUFBQUFBQUVBQmlabW9MaUFRQUFBQUFBQUFJQVZJcmpBUUFBQUFBQUFBWUF6Wm96a0M2QTVRRUFBQUFBQUFBQ0FHYVg1d0VBQUFBQUFBQUdBRXFJczRFWW9la0JBQUFBQUFBQUFnRHNtK29CQUFBQUFBQUFBZ0IvaytzQkFBQUFBQUFBQkFBM2lXQ3I3QUVBQUFBQUFBQUVBTGFSeTVmdUFRQUFBQUFBQUFJQSs2ZnZBUUFBQUFBQUFBZ0FiWURsallHTWw0dnhBUUFBQUFBQUFBWUFCSjh1Z2FPUzlBRUFBQUFBQUFBR0FDQ0lyb2twcVBjQkFBQUFBQUFBQkFDU2ovbUMrQUVBQUFBQUFBQUVBRFdicjUvNkFRQUFBQUFBQUFJQWxaejhBUUFBQUFBQUFBSUFENlBvQVFBQUFDQUFBQWdBcEtrRmh2MkZMSUxwQVFBQUFDQUFBQW9BY0pkemhGeUNsNE5Wa3VzQkFBQUFJQUFBQkFCTG5uMkM3QUVBQUFBZ0FBQUNBRXlBN1FFQUFBQWdBQUFDQU5LWTdnRUFBQUFnQUFBQ0FCV3k3d0VBQUFBZ0FBQUVBTGlGTHFUd0FRQUFBQ0FBQUFRQTRhZXNodkVCQUFBQUlBQUFDQUNvbmlXR3hveFVnL0lCQUFBQUlBQUFDQUFLZ3dHRCtvTExyL01CQUFBQUlBQUFBZ0JjcS9RQkFBQUFJQUFBQkFDL2hBeU85UUVBQUFBZ0FBQUVBRDZJRUp6MkFRQUFBQ0FBQUFvQUtZU3NnRVNIVVlDRWlmY0JBQUFBSUFBQUJnQk9naldZYjV6NEFRQUFBQ0FBQUFJQVU0djdBUUFBQUNBQUFBUUFaSy9zaGZ3QkFBQUFJQUFBREFCaWdOR0kzNE9Xa1hpQnBKVDlBUUFBQUNBQUFBSUFPNDcrQVFBQUFDQUFBQVlBdW9na24yZVcvd0VBQUFBZ0FBQUNBSjI0QUFJQUFBQWdBQUFFQU1lYVdvQUJBZ0FBQUNBQUFBWUFJNXhRamNLR0FnSUFBQUFnQUFBSUFBS0ZnNUV1bDkyUkF3SUFBQUFnQUFBRUFQZUFZYW9FQWdBQUFDQUFBQVFBam9QV293VUNBQUFBSUFBQUFnQ1Zwd1lDQUFBQUlBQUFBZ0JOcVFnQ0FBQUFJQUFBQkFCZ29qeVdDUUlBQUFBZ0FBQUdBTTZJTlkvZWxBb0NBQUFBSUFBQUJnQ2NnQ0daUkpZTEFnQUFBQ0FBQUFRQUo2WHZqQXdDQUFBQUlBQUFCQURuZ0xpNkRnSUFBQUFnQUFBR0FBYURiTFJVaHc4Q0FBQUFJQUFBQ2dCQWp4ZUhQb3NVbTNtQyIsInJhbmdlIjp7Im1pbiI6IiIsIm1heCI6IjA1QzFBMCJ9fV0%3D

CaitlinV39 commented 3 years ago

Thanks @mikemassa84. We are able to reproduce and are considering what the best next step would be. We did reduce the size of the token, but when we base64 encode it, it is adding enough that it is getting pushed over the limit. I'll reopen this issue.

mikemassa84 commented 3 years ago

@CaitlinV39 maybe a hash and store? You could put the base64 string through a sha256 hash and build a hex string out of it. That'd consistently give you a 64 character string you could use in place of the base64 string. You'd have to store the hash to original string mapping though to get back to your original token...

shahedaAnsari commented 3 years ago

we are also facing same issue and we are using 2.0.172 build

CraigP68 commented 2 years ago

AB#87261

EXPEkesheth commented 1 year ago

@mikemassa84 - we recently addressed the issue by introducing header "x-ms-documentdb-responsecontinuationtokenlimitinkb". The next link in the bundle has a continuation token size limit of 3KB. You have flexibility to tweak the continuation token size between 1 to 3KB using header. Please let us know if your issue is addressed with the header.