Getting one company from two tables [closed]

Question

Currently I'm working with fastapi, sqlalchemy to connect to multiple databases. It checks params to ensure it has company_name, then will return the current table in that database, but if no company_name is set then it will return one record from each Table in the 2 databases.

class Params(BaseModel):
    company_name: str | None


def multiple_db(db, model):
    # return data from db TEST1
    ret = db.query(model).all()

    # return data from db TEST2
    ret1 = db.query(model).all()

    return ret, ret1


def db_query(db: Session, params, model):
    if params.company_name == "TEST1":
        return db.query(model)

    if params.company_name == "TEST2":
        return db.query(model)


@app.get("/")
def get_all(params: Params = Depends(), db: Session = Depends(get_db)):
    if params.company_name == None:
        ret, ret1 = multiple_db(db, MODEL1, MODEL2)
        return {"ret": ret, "ret2": ret1}

    else:
        ret = db_query(db, params, MODEL1).all()
        return ret

Is there a way to improve this code?

Answer 1

usual type hinting

    company_name: str | None

This is a perfectly nice declaration, which you can leave as-is. But prefer Optional when using a modern cPython interpreter.

from typing import Optional
...
    company_name: Optional[str]

Hinting in the python community has been an evolving practice. Let's take Y to be the type None. Despite the goal of

There should be one -- and preferably only one -- obvious way to do it.

we have the Union[X, Y], X | Y, and Optional[X] synonyms. The first is a verbose historical artifact which is discouraged in newly authored source code -- prefer the second, e.g. float | complex. In the special case of Y being None, prefer Optional, as an aid to human cognition. It's just how folks reading your code will think about the parameter. We see similar effects in other languages, such as using ? punctuation to guard against a null de-ref. It's technically precise notation for the machine, which doesn't get in the way of reasoning about the happy-path of an algorithm.

Consider making the name mandatory (that is, a varchar which is NOT NULL).

EDIT

There's a lot of "six versus half-dozen" when deciding to express a type as Optional[x] versus x | None. Prior to python 3.10 there wasn't much of a choice -- who would want a clunky Union[x, None] ? Apparently I've been working with a lot of pre-3.10 hinted source code. I withdraw the company_name remark.

I defer to Martijn Pieters, who advises to prefer x | None.

---

use meaningful identifiers

    return ret, ret1

Those are not great names, especially since database TEST2 is associated with ret1. Prefer return rows1, rows2 or something more verbose that mentions the DB names. Same issue crops up when you return {"ret": ret, "ret2": ret1}. Definitely choose a more informative identifier than "ret" when defining a public API which web clients will consume.

Also, the code appears to query same DB twice, despite the comment's claim that we query a pair of databases. Similar remark further down within def db_query.

Type hinting the signature wouldn't hurt.

We appear to return a pair of generators, which can be confusing to a caller that anticipates getting a container which can be scanned more than once. Consider wrapping the .all() result with list( ... .all() )

---

mutable default arg

def get_all(params: Params = Depends(), db: Session = Depends(get_db)):

OP omits the import / definition for Depends.

Avoid evaluating such expressions at import time. Prefer a default value of None, and then assign db = db or Depends(get_db) down in the method body. That will be evaluated at run time rather than at import time, side stepping a whole category of tricky bugs. Even if it doesn't matter in this particular code, avoid the bad habit. Use a linter to help you out. For example pylint will report that as "W0102: Dangerous default value".

    if params.company_name == None:

nit: prefer is over == when testing for that particular singleton. Again, rely on a linter, which might report "C0121 (singleton-comparison)".

EDIT

Hmmm, I use sqlalchemy a lot, but not fastapi. It appears the omitted import was

from fastapi import Depends

The docs seem to encourage mutable default args. I am unfamiliar with the fastapi details, and do not yet have an opinion on best way to express that. Sticking to what the docs recommend seems best ATM.

---

callsite should match signature

        ... = multiple_db(db, MODEL1, MODEL2)

This appears to be correct, in the sense that it matches Author's Intent revealed in the comments. It specifies one more parameter than the def signature supports.

---

add hints

def db_query(db: Session, params, model):

Thank you for the Session type hint.

This signature should end with -> and the intended return type. If it did, then mypy would helpfully tell you about the implicit return None at end of function.

If caller didn't pass in one of the two valid values, then better to raise a fatal error than silently return a value of incorrect type.

---

relational design

Consider defining a VIEW that offers UNION ALL of the rows of both tables.

Then all queries are against the view, and your WHERE clause determines how many tables participate in forming the result set.

---

This codebase does not appear to achieve its design goals.

I would not be willing to delegate or accept maintenance tasks on it.