Missing port in connection string results in server crash

[domdomegg]

Description

When configuring Mathesar, I set MATHESAR_DATABASES to something like (db|postgresql://u:[email protected]/db)

Actual behavior

Server crashes on attempting to connect to this database, with the stacktrace:

Stacktrace

Traceback (most recent call last):
  File "/code/install.py", line 49, in <module>
    main()
  File "/code/install.py", line 25, in main
    management.call_command('migrate')
  File "/usr/local/lib/python3.9/site-packages/django/core/management/__init__.py", line 168, in call_command
    return command.execute(*args, **defaults)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/base.py", line 371, in execute
    output = self.handle(*args, **options)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/base.py", line 85, in wrapped
    res = handle_func(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/django/core/management/commands/migrate.py", line 267, in handle
    emit_post_migrate_signal(
  File "/usr/local/lib/python3.9/site-packages/django/core/management/sql.py", line 48, in emit_post_migrate_signal
    models.signals.post_migrate.send(
  File "/usr/local/lib/python3.9/site-packages/django/dispatch/dispatcher.py", line 177, in send
    return [
  File "/usr/local/lib/python3.9/site-packages/django/dispatch/dispatcher.py", line 178, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/code/mathesar/apps.py", line 16, in _prepare_database_model
    make_sure_initial_reflection_happened()
  File "/code/mathesar/state/base.py", line 8, in make_sure_initial_reflection_happened
    reset_reflection()
  File "/code/mathesar/state/base.py", line 27, in reset_reflection
    _trigger_django_model_reflection(db_name)
  File "/code/mathesar/state/base.py", line 31, in _trigger_django_model_reflection
    reflect_db_objects(metadata=get_cached_metadata(), db_name=db_name)
  File "/code/mathesar/state/django.py", line 38, in reflect_db_objects
    sync_databases_status(databases)
  File "/code/mathesar/state/django.py", line 59, in sync_databases_status
    db._sa_engine.connect()
  File "/code/mathesar/models/base.py", line 129, in _sa_engine
    engine = create_mathesar_engine(db_name=db_name)
  File "/code/mathesar/database/base.py", line 22, in create_mathesar_engine
    return engine.create_future_engine_with_custom_types(**credentials)
  File "/code/db/engine.py", line 12, in create_future_engine_with_custom_types
    engine = create_future_engine(
  File "/code/db/engine.py", line 25, in create_future_engine
    conn_url = URL.create(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/url.py", line 151, in create
    cls._assert_port(port),
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/url.py", line 162, in _assert_port
    return int(port)
ValueError: invalid literal for int() with base 10: ''

Note: it appears to only do this for the MATHESAR_DATABASES option. In DJANGO_DATABASE_URL the port can be omitted any everything works fine, which makes this error more confusing to users.

Expected behavior

I expected the connection to work, as the connection string had been copied from another application that was working (which was assuming a 5432 default postgres port).

Alternatively in hindsight, I would have expected a clearer error message stating the port was mandatory (maybe ideally with the web server stating this to help users debug it quickly).

To Reproduce

Set MATHESAR_DATABASES to something like (db|postgresql://u:[email protected]/db)

Environment

Mathesar 0.1.3

Additional context

This is low priority for us, given we've now figured it out. But maybe helpful to fix to simplify onboarding for new users.

[seancolsen]

Thanks for reporting this, @domdomegg. Sorry you experienced a bit of friction here. I agree that we should infer 5432 if the port is missing, as that would lead to a smoother experience in cases like yours.

Interestingly though, in Mathesar 0.1.4 we are moving the entire database connection configuration into the UI which I think will obviate the friction you experienced. For example, as in this mockup of the "New Connection" form, the "Port" field will be required and automatically filled with 5432.

We're still not entirely sure what level of support we'd like to offer for administrators to configure connections through the MATHESAR_DATABASES environment variable, as you've been doing. We may choose to retain that functionality, in which case this issue you reported would still be relevant. However, we might remove the MATHESAR_DATABASES entirely, at which point this issue would be moot.

I'm going to leave this open for now but mark it for discussion at our January "Product approval" meeting where we'll help determine the next steps.

[domdomegg]

Sure. If it helps, I'd be fine with configuring the Mathesar databases from the UI and dropping MATHESAR_DATABASES entirely (although it is kinda nice at the moment that it can be provisioned and configured with my IaC tools alone via env variables, this isn't critical).

[mathemancer]

Related: #3355

The current plan, as I see it, is to remove the documentation of this variable from the user-facing docs, since the main use of it moving forward is simply to allow for setting up test DBs when running our test suite. I think that should resolve any confusion for new users.

For already-existing users who may be using the variable, we may need to add some kind of note to warn them that changes to that variable will not have expected results w.r.t. their installation's DB connections. (Such modifications always had odd results; part of the reason for removing the variable).

Once we have the test suite sorted without that variable, we'll likely deprecate it, then remove support entirely.

[seancolsen]

@domdomegg

We discussed this briefly today, just to make sure our plan is solid.

• We still need to maintain support for the MATHESAR_DATABASES environment variable because our test suite relies on it. But it won't be a user-facing feature.

• Starting in v0.1.4, the MATHESAR_DATABASES variable will work like this:

  • When Mathesar starts, it will read connection strings from MATHESAR_DATABASES and copy those connection credentials into the internal database. They'll remain in the internal database, as if the user had configured them through the UI. They'll be editable through the UI, but it's possible that if the user edits some part of the connection details through the UI, then Mathesar will create a duplicate connection on restart if it reads that original environment variable again.

• We will be making sure that our user docs instruct admins to remove MATHESAR_DATABASES after upgrading to v0.1.4. And aside from that, we'll make sure that our user docs don't mention MATHESAR_DATABASES.

• We hear you on the convenience of using environment variables for provisioning and configuring Mathesar installations. If you end up finding our new UI-only approach to be a burden, then please open a new ticket where we can discuss how to better meet your needs (while still allowing other users to configure connections via the UI). We're open to doing more work towards to help you with configuration, but we'll need to get clearer on the underlying goals.

Because MATHESAR_DATABASES will be a developer-facing feature only (for the test suite), I'm assuming we can all agree that requiring an explicit port number is appropriate in that context. For this reason, I'm closing this ticket now.