qfieldCloud improvements.md

qfieldCloud improvements.md

1. Mutable default argument in permission_check

https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html

File: docker-app/qfieldcloud/core/permission_check.py, line 9

The check_args parameter uses a mutable list [] as its default value. While it doesn't seem to be mutated in the current code, this is a common Python footgun — if anyone ever adds an in-place modification down the line, the shared list would cause hard-to-trace bugs across decorated views.

Current code:

def permission_check(perm: str, check_args: list[str | Callable] = []) -> Callable:

Suggested fix:

def permission_check(perm: str, check_args: list[str | Callable] | None = None) -> Callable:
    if check_args is None:
        check_args = []

---

2. Class-level mutable list on QfcMultiPartSerializer

File: docker-app/qfieldcloud/core/views/files_views.py, line 227

errors is defined as a class variable (errors: list[str] = []), which means all instances share the same list. Since parse() calls self.errors.append(...), errors from one request leak into subsequent requests. In a multi-threaded Django server this could also cause race conditions.

Current code:

class QfcMultiPartSerializer(MultiPartParser):
    errors: list[str] = []

    def parse(self, stream, media_type=None, parser_context=None) -> DataAndFiles:
        parsed = super().parse(stream, media_type, parser_context)
        if "file" not in parsed.files or not parsed.files["file"]:
            self.errors.append(...)  # modifies the shared class-level list
        return parsed

Suggested fix:

class QfcMultiPartSerializer(MultiPartParser):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.errors: list[str] = []

    def parse(self, stream, media_type=None, parser_context=None) -> DataAndFiles:
        parsed = super().parse(stream, media_type, parser_context)
        if "file" not in parsed.files or not parsed.files["file"]:
            self.errors.append(...)
        return parsed

Note: The post() method in DownloadPushDeleteFileView references QfcMultiPartSerializer.errors (class-level access) when reporting to Sentry. That call site would need updating too, otherwise it would read a fresh empty class list instead of the instance's errors. So most likely this is a design decision.

---

3. Bare except Exception hides unexpected errors

File: docker-qgis/qfc_worker/utils.py, around line 88

The code checks whether Qgis.NoLevel exists and falls back to setting it manually. The intent is clearly to catch AttributeError, but except Exception would also swallow unrelated errors and make debugging harder.

Current code:

try:
    Qgis.NoLevel
except Exception:
    Qgis.NoLevel = 4

Suggested fix:

try:
    Qgis.NoLevel
except AttributeError:
    Qgis.NoLevel = 4

---

4. Implicit None return in wait_for_postgres

File: docker-app/wait_for_services.py

If the while loop exits without a successful connection and without hitting the except branch's timeout path, the function falls through without a return statement, implicitly returning None instead of False.

Current code (simplified):

def wait_for_postgres():
    start_time = time()
    while time() - start_time < TIMEOUT:
        try:
            conn = psycopg2.connect(**config)
            conn.close()
            return True
        except psycopg2.OperationalError as error:
            if time() - start_time < TIMEOUT:
                sleep(INTERVAL)
            else:
                logger.error(...)
                # no return here

    logger.error(f"Could not connect to Postgres within {TIMEOUT} seconds.")
    # falls through — returns None

Suggested fix:

def wait_for_postgres() -> bool:
    start_time = time()
    while time() - start_time < TIMEOUT:
        try:
            conn = psycopg2.connect(**config)
            conn.close()
            return True
        except psycopg2.OperationalError as error:
            if time() - start_time < TIMEOUT:
                sleep(INTERVAL)
            else:
                logger.error(...)
                return False

    logger.error(f"Could not connect to Postgres within {TIMEOUT} seconds.")
    return False

---

5. bool() on query string gives wrong result for "false"

File: docker-app/qfieldcloud/core/views/files_views.py, lines 110–114

The skip_metadata query parameter is parsed with bool(skip_metadata_param), but in Python bool("false") evaluates to True (any non-empty string is truthy). So a client sending ?skip_metadata=false would actually enable metadata skipping.

Current code:

skip_metadata_param = request.GET.get("skip_metadata", "0")
if skip_metadata_param == "0":
    skip_metadata = False
else:
    skip_metadata = bool(skip_metadata_param)  # bool("false") == True

Suggested fix:

skip_metadata_param = request.GET.get("skip_metadata", "0")
try:
    skip_metadata = bool(int(skip_metadata_param))
except (ValueError, TypeError):
    skip_metadata = False

Or, if the API should be more forgiving with string values, a small helper could handle "true"/"false"/"yes"/"no"/"1"/"0" consistently.

---