Skip to content

Commit

Permalink
#57
Browse files Browse the repository at this point in the history
  • Loading branch information
@vlad-ignatov
vlad-ignatov committed Jul 26, 2021
1 parent e13e49f commit 92fec07
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 21 deletions.
3 changes: 2 additions & 1 deletion src/simple-proxy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,8 @@ module.exports = (req, res) => {
});

if (!isBinary) {
stream = stream.pipe(replStream(fhirServer, `${config.baseUrl}/v/${fhirVersionLower}/fhir`));
// stream = stream.pipe(replStream(fhirServer, `${config.baseUrl}/v/${fhirVersionLower}/fhir`));

This comment has been minimized.

Sign in to view

Copy link
@barabo

barabo Jul 26, 2021

is this a related change?

This comment has been minimized.

Sign in to view

Copy link
@vlad-ignatov

vlad-ignatov Jul 26, 2021 •
edited
Loading

Author Contributor

Yes. This is to fix what Josh commented regarding the random port on which the tests are running. I don't want to run them on fixed port because that is how it used to be and I had a lot of issues (AFAICR supertest was still starting another server on random port which wasn't being used but also caused test to not exit...). This makes it so that the proxy "translates" links to whatever the current origin is, instead of using the pre-configured config.baseUrl.

On the other hand, this change is not 100% related to this test any more because the next link request is now relative and the host/port part is ignored. However, I still think it was the right thing to do.

P.S. I'm not a big fan of config.baseUrl. It simplifies the code but also causes all sorts of problems and I think that ideally, the baseURL should always be auto-detected. I am not saying that config.baseUrl is deprecated, but in my mind it is. Perhaps I will manage to remove it some day, so please avoid using it if possible.

This comment has been minimized.

Sign in to view

Copy link
@barabo

barabo Jul 26, 2021

That all sounds good, thanks for the background info. However, it does look extraneous because of the commented-out code.
stream = stream.pipe(replStream(fhirServer, `${Lib.getRequestBaseURL(req)}/v/${fhirVersionLower}/fhir`));
}

stream.pipe(res);
Expand Down
43 changes: 23 additions & 20 deletions test/fhir-servers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,7 @@ for(const FHIR_VERSION in TESTED_FHIR_SERVERS) {
});

it ("Handles pagination", () => {
return agent.get(`${PATH_FHIR}/Patient`)
.expect(res => {
return agent.get(`${PATH_FHIR}/Patient`).expect(async (res) => {
if (!Array.isArray(res.body.link)) {
throw new Error("No links found");
}
Expand All @@ -284,26 +283,30 @@ for(const FHIR_VERSION in TESTED_FHIR_SERVERS) {
if (!next) {
throw new Error("No next link found");
}
// console.log(next)
return agent.get(next.url).expect(res2 => {
if (!Array.isArray(res.body.link)) {
throw new Error("No links found on second page");
}

const nextURL = new URL(next.url)

const res2 = await request(app).get(nextURL.pathname + nextURL.search)

This comment has been minimized.

Sign in to view

Copy link
@barabo

barabo Jul 26, 2021

Why not agent here instead of request(app)?

This comment has been minimized.

Sign in to view

Copy link
@vlad-ignatov

vlad-ignatov Jul 26, 2021

Author Contributor

Thanks! Because I was trying things and I forgot that one as is. It shouldn't matter that much because agent is just a shortcut, but I will have that changed in second.

if (!Array.isArray(res2.body.link)) {
throw new Error("No links found on second page");
}

let self = res.body.link.find(l => l.relation == "self")
if (!self) {
throw new Error("No self link found on second page");
}
if (self.url !== next.url) {
throw new Error("Links mismatch");
}
let self = res2.body.link.find(l => l.relation == "self")

if (!self) {
throw new Error("No self link found on second page");
}

if (self.url !== next.url) {
throw new Error("Links mismatch");
}

let next2 = res.body.link.find(l => l.relation == "next")
if (!next2) {
throw new Error("No next link found on second page");
}
// console.log(next2)
})
let next2 = res.body.link.find(l => l.relation == "next")

if (!next2) {
throw new Error("No next link found on second page");
}
})
});

Expand Down

1 comment on commit 92fec07

@barabo
Copy link

@barabo barabo commented on 92fec07 Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - your changes fit in better with the existing codebase, so I'm fine if you want to commit it directly and I'll just delete my fix branch. Thanks!

Please sign in to comment.