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

Relative DB URLs #1319

Merged
merged 41 commits into from
Jul 28, 2022
Merged

Relative DB URLs #1319

merged 41 commits into from
Jul 28, 2022

Conversation

TheSlimvReal
Copy link
Collaborator

@TheSlimvReal TheSlimvReal commented Jun 24, 2022

solves #841
On default the app will now be in demo_mode with a in-memory DB. Do change this DEMO="false" needs to be passed to the Docker image on start.

Visible/Frontend Changes

--

Architectural/Backend Changes

  • DB is only accessed through relative URL which makes the database_url in the config.json file unnecessary
  • Replace database_name with default app
  • Demo mode and session type can be configured through environment or query params
  • Dockerfile allows to add a default redirect to demo mode for all requests
  • Site name has been move to config in database

@TheSlimvReal TheSlimvReal changed the title Relative url Relative DB URLs Jun 24, 2022
@github-actions
Copy link

@sleidig
Copy link
Member

sleidig commented Jun 24, 2022

For me, one of the main objectives of removing the config.json is to have a multi-tenant system (one hosted app for all projects). Do we have an idea yet how to route to different DBs in the long run?

And if we remove the url, we not also move the name to the config document in couchdb and immediately delete the whole json file?

@TheSlimvReal
Copy link
Collaborator Author

@sleidig

Regarding the multi tenant system:
I would expect the URL to the DB of a user would be provided through the authentication process. E.g. keycloak would send the DB URL after a successful login.

Regarding the full deletion of the config.json file:
It still holds the information for the session and demo mode. For this I don't have a clear solution yet how we would otherwise configure this as it cannot come from the database.

@sleidig sleidig marked this pull request as draft July 5, 2022 13:59
@TheSlimvReal TheSlimvReal marked this pull request as ready for review July 8, 2022 11:56
@TheSlimvReal TheSlimvReal marked this pull request as draft July 8, 2022 12:20
TheSlimvReal and others added 10 commits July 11, 2022 17:58
# Conflicts:
#	build/Dockerfile
#	build/default.conf
#	src/app/app.module.ts
#	src/app/core/config/config-fix.ts
#	src/app/core/ui/ui-config.ts
#	src/main.ts
# Conflicts:
#	build/default.conf
#	src/app/core/language/language-select/language-select.component.html
#	src/app/core/translation/language-selector/language-select.component.ts
@TheSlimvReal TheSlimvReal marked this pull request as ready for review July 19, 2022 17:22
@TheSlimvReal TheSlimvReal requested a review from sleidig July 19, 2022 17:22
Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Wow, I really like how this cleans up some long standing loose ends 😃
Just a bunch of clean code ideas in my comments but this looks pretty good overall.

Functionally I noticed one thing:
When I switch to "local" session and then do a hard reload (Ctrl+F5) the app gets the queryParam "session=mock" in the URL and switches back to "mock" session. Looking at the code I see that queryParam takes precedence over localStorage by design.

My intuitive expectation was that I will remain in the previous (-> local) mode. What should we have as the expected behaviour here?

build/Dockerfile Outdated Show resolved Hide resolved
}
if ($demo = "truetrue") {
# This first part is required because Heroku does some internal redirects which break the rewrite otherwise
# This does not work in localhost as it enforces HTTPS
Copy link
Member

Choose a reason for hiding this comment

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

do we have to rewrite to https here or could we just leave it at whatever was used before?

It would make more sense to have a separate block that rewrites any mode to use https (easier to understand if separated - and applying to all modes). But I don't know whether that's doable in nginx, e.g. multiple rewrite blocks applying.

Copy link
Collaborator Author

@TheSlimvReal TheSlimvReal Jul 20, 2022

Choose a reason for hiding this comment

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

Normally it definitely works the way you are asking for the problem is that it does not work with Heroku. Somehow in there a internal redirect is happening before this one which leads to a wrong URL in this rewrite (in particular loosing the https and adding a wrong port). This workaround is not needed for our own servers. There we could just use:

rewrite ^ $uri?demo=true&session=mock permanent;

build/default.conf Outdated Show resolved Hide resolved
src/app/core/app-config/app-config.ts Outdated Show resolved Hide resolved
src/app/core/app-config/app-config.ts Outdated Show resolved Hide resolved
src/app/core/config/config-fix.ts Outdated Show resolved Hide resolved
src/app/core/app-config/app-config.ts Outdated Show resolved Hide resolved
src/assets/config.default.json Outdated Show resolved Hide resolved
src/environments/environment.ts Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Jul 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

looks awesome! 😍

@TheSlimvReal TheSlimvReal merged commit ca3704a into master Jul 28, 2022
@TheSlimvReal TheSlimvReal deleted the relative-url branch July 28, 2022 16:06
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.9.0-master.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

3 participants