Daylily-Informatics / bloom

Templated Abstract Polymorphic (and opinionated) LIMS
Mozilla Public License 2.0
0 stars 1 forks source link

Do not use mutable objects in method params #10

Open adamtracy opened 9 months ago

adamtracy commented 9 months ago

Remove mutable objects from method param defaults

noticed this while debugging pregen_AY.py in bdb.py:607 def create_instance(self, template_euid, json_addl_overrides={}): and how this was affecting the data while stepping _create_child_instance(). It's going to lead to data corruption and is partly why i'm gettting that traceback (i think). here's GPT's explanation

--

You are correct. In Python, using mutable objects like lists ([]) or dictionaries ({}) as default arguments in a function or method definition is generally considered a bad practice. This is due to the way Python handles default argument values, which can lead to confusing and often undesirable behaviors.

The Issue with Mutable Default Arguments

When you use a mutable object as a default argument in Python, the default value is created only once when the function is defined, not each time the function is called. This means that if the mutable object is modified (like adding an item to a list or changing a value in a dictionary), the modified object will be used as the default value for subsequent calls to the function. This can lead to bugs that are difficult to diagnose.

Example of the Issue

def append_to_list(element, my_list=[]):
    my_list.append(element)
    return my_list

# You might expect these calls to return lists containing only one element each
print(append_to_list(1))  # Output: [1]
print(append_to_list(2))  # Output: [1, 2] - but it actually accumulates values

Better Approach: Using None as a Default

A better approach is to use None as the default value and then check for this in the function body to create a new mutable object if necessary.

def create_instance(self, template_euid, json_addl_overrides=None):
    if json_addl_overrides is None:
        json_addl_overrides = {}
    # Now you can safely use json_addl_overrides

In this revised version, a new dictionary is created each time create_instance is called without a json_addl_overrides argument, avoiding the unintended sharing of the same dictionary across calls.

By following this practice, you ensure that each call to the function gets its own independent mutable object, which is usually the expected behavior.

adamtracy commented 9 months ago

Curious what sqlalchemy you're running with @iamh2o

(BLOOM) ants-MacBook-Pro:bloom ant$ conda list sqlalchemy
# packages in environment at /Users/ant/miniconda3/envs/BLOOM:
#
# Name                    Version                   Build  Channel
sqlalchemy                2.0.23          py312he37b823_0    conda-forge
adamtracy commented 9 months ago

Also, @iamh2o what postgres version are you using?

postgres=> select version();
-[ RECORD 1 ]-------------------------------------------------------------------------------------------------------------
version | PostgreSQL 16.1 on x86_64-apple-darwin20.6.0, compiled by Apple clang version 12.0.5 (clang-1205.0.22.9), 64-bit
adamtracy commented 9 months ago

the actual problem here was a result of using envsubst on bloom_lims/env/postgres_schema_v3.sql where the update_record() trigger has an evaluated code fragment using $1. envsubst replaces this with '' since $1 isn't a real env var. there's a better way to handle all this but i dont think it's a priority at the moment.