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

fix: API returns invalid file URLs, due to including the port #2223

Merged
merged 2 commits into from Sep 29, 2022
Merged

fix: API returns invalid file URLs, due to including the port #2223

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82529cf
make sipi internal base url a lazy val
BalduinLandolt Sep 28, 2022
85aefc3
make IIIF url depend on protocol env variable
BalduinLandolt Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 4 additions & 6 deletions webapi/src/main/scala/org/knora/webapi/config/AppConfig.scala
View file
Open in desktop
Expand Up @@ -134,12 +134,10 @@ final case class Sipi(
audioMimeTypes: List[String],
archiveMimeTypes: List[String]
) {
val internalBaseUrl: String = "http://" + internalHost + (if (internalPort != 80)
":" + internalPort
else "")
val externalBaseUrl: String = "http://" + externalHost + (if (externalPort != 80)
":" + externalPort
else "")
val internalBaseUrl: String =
internalProtocol + "://" + internalHost + (if (internalPort != 80) ":" + internalPort else "")
val externalBaseUrl: String =
externalProtocol + "://" + externalHost + (if (externalPort != 80) ":" + externalPort else "")
Comment on lines +137 to +140
Copy link
Collaborator

@mpro7 mpro7 Sep 29, 2022 •
edited

Choose a reason for hiding this comment

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

Suggested change
val internalBaseUrl: String =
internalProtocol + "://" + internalHost + (if (internalPort != 80) ":" + internalPort else "")
val externalBaseUrl: String =
externalProtocol + "://" + externalHost + (if (externalPort != 80) ":" + externalPort else "")
val internalBaseUrl: String = internalProtocol + "://" + internalHost
val externalBaseUrl: String = externalProtocol + "://" + externalHost

Is the port needed here at all? If not then it should be shortened as above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hard to say, it might break locally, or in tests... But even if so, I do suspect that externalPort != 80 does not make sense, it should be 1024 if anything, because that's the config default, which should only happen when there is no environment variable set (and it's also what the docker-compose specifies, when ewe run it locally).
But I am reluctant to change these things now: Then we might not know what actually fixed the but. So IMO it would be best to only change what we expect to be the core issue, and then do a separate cleanup of all this stuff in a dedicated task. The same should also be done with all other configs, not only SIPI - but then we'll have to do much more testing, so we can't deploy the fix to prod anytime soon... Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That means you are guessing a lot, but if you feel there is no other way to make sure what and if it works, then go ahead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes... it's a guessing fix at that point (though there is a good chance this will actually fix it, we did do some testing). But the guessing thing only gets harder when we change more things, so I'd actually prefer it like this, I think.

val timeoutInSeconds: duration.Duration = scala.concurrent.duration.Duration(timeout)

}
Expand Down