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.

Sign up for GitHub

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

Jump to bottom

Instance-level history fullUrl is wrong when resource id begins with resource type name #3501

Closed
lmsurpre opened this issue Mar 24, 2022 · 3 comments
Closed

Instance-level history fullUrl is wrong when resource id begins with resource type name #3501

lmsurpre opened this issue Mar 24, 2022 · 3 comments
Assignees
@lmsurpre
Labels
bug Something isn't working

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Mar 24, 2022 •
edited
Loading

Describe the bug
While testing instance-level history, I noticed that the Bundle.entry.fullUrl we are returning are duplicating the resource type in their paths when the resource id starts with the resource type name (e.g. "Patient1")

Looks like this is the same as #2965 except for fullUrls (instead of location URIs)

Environment
main

To Reproduce
Steps to reproduce the behavior:

  1. create a resource on the server with an id that begins with a resource type name (e.g. Patient/Patient1)
  2. retrieve the instance-level history (e.g. Patient/Patient1/_history)

Note that the fullUrl contains an extra type in the path (e.g. https://localhost:9443/fhir-server/api/v4/Patient/Patient/Patient1)

Expected behavior
The fullUrl should be the correct URL to the resource

Additional context
I also noticed a couple other differences between our instance-level history and the new system and type-level history endpoints.

  1. instance-level history returns a fullUrl that includes the full base url of the server whereas our new system and type-level history responses which include only a relative URL.
  2. instance-level history includes (Response.status, Response.etag, and Response.lastModified) whereas the system and type-level history responses include (Response.status, Response.location, and Response.lastModified)
  3. instance-level history sets Response.status to "200" for creates; system and type-level history set these to "201 Created"

For number 1, I actually think the instance-level interaction is more correct... @punktilious how costly would it be to return full url instead of relative url in our system and type-level history responses?

For number 2, I think we should add Response.location to the instance-level response. Maybe a separate ticket?

For number 3, I think we should modify the instance-level response to use 201 for creates to match. We should also be consistent as to whether we include just the numerical HTTP response code ("200") or also include the response code title ("200 OK"). Separate issue?
@lmsurpre lmsurpre added the bug Something isn't working label Mar 24, 2022
@lmsurpre
Copy link
Member Author

pointed to account for the changes in "additional context" as well

@lmsurpre lmsurpre self-assigned this Mar 24, 2022
@lmsurpre lmsurpre changed the title Instance-level history fullUrl have duplicate resource type Instance-level history fullUrl is wrong when resource id begins with resource type name Mar 24, 2022
lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
93ea3f7 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
c7af108 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
5f97f94 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member Author

lmsurpre commented Mar 24, 2022 •
edited
Loading

For number 3 (200 vs 200 OK) I checked our current batch/transaction processing code and we use the number without the string there, so lets normalize on that (i.e. change the system-level history to match)

lmsurpre added a commit that referenced this issue Mar 24, 2022
@lmsurpre
issue #3501 - use absolute URLs in the system/type history response
4271a48 
For both `Entry.fullUrl` and `Entry.response.location`.

I also added the etag element to the Entry.response objects to be more
consistent with the instance-level history implementation.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
@lmsurpre
issue #3501 - reconcile minor difference between history apis
c2e12af 
1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre lmsurpre mentioned this issue Mar 25, 2022
Merged
lmsurpre added a commit that referenced this issue Mar 25, 2022
@lmsurpre
issue #3501 - apply fix for #2965 more universally
43a14de 
We had duplicate logic in two classes:
* FHIRResource.getRequestBaseUri
* FHIRRestHelper.getRequestBaseUri

For #2965, we fixed the version in FHIRResource but missed the one in
FHIRRestHelper.

Now I combined the two implementations into FHIRRestHelper, made it
static, and removed the version from FHIRResource.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
@lmsurpre
issue #3501 - use absolute URLs in the system/type history response
bf9296c 
For both `Entry.fullUrl` and `Entry.response.location`.

I also added the etag element to the Entry.response objects to be more
consistent with the instance-level history implementation.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
@lmsurpre
issue #3501 - reconcile minor difference between history apis
d4a5a5f 
1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 25, 2022
@lmsurpre
issue #3501 - reconcile minor difference between history apis
6c65652 
1. add Entry.response.location to instance-history response
2. set 201 for the initial create entry (we need #3507 for the undelete
case)
3. remove the status code name from Entry.response.status for system and
type-level history interactions (e.g. `200 OK` -> `200`)

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Mar 30, 2022
@lmsurpre
Merge pull request #3506 from IBM/issue-3501
8907021 
issue #3501 - apply fix for #2965 more universally
@d0roppe
Copy link
Collaborator

d0roppe commented Apr 1, 2022

verified that the _history full url is now correct with a couple of different resources. Closing issue

@d0roppe d0roppe closed this as completed Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmsurpre lmsurpre

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lmsurpre @d0roppe