Code & data model audit

Datamodel introspection

schema qwat_dr:

schema qwat_od

schema qwat_vl

schema qwat_sys

Because the tool used to generate the previous diagrams (postgresql_autodoc) cannot represent inherited relationships Here is how to find them in sql :

SELECT parnsp.nspname AS par_schemaname
    , parcla.relname AS par_tablename
    , chlnsp.nspname AS chl_schemaname
    , chlcla.relname AS chl_tablename
 FROM pg_catalog.pg_inherits
 JOIN pg_catalog.pg_class AS chlcla ON (chlcla.oid = inhrelid)
 JOIN pg_catalog.pg_namespace AS chlnsp ON (chlnsp.oid = chlcla.relnamespace)
 JOIN pg_catalog.pg_class AS parcla ON (parcla.oid = inhparent)
 JOIN pg_catalog.pg_namespace AS parnsp ON (parnsp.oid = parcla.relnamespace)

it shows that only qwat_vl.value_list_base is used as a parent table of 37 children in the qwat_vl schema.

Network elements

It appears that there is no true inheritance for network elements. There are just id shared between tables.

Although the comment on network_element table says that it inherits from node, there's not foreign key between them, which seems dangerous for model integrity.

Integrity seems to be delegated to triggers that does not prevent against modifications on some tables.

For example nothing prevents us to delete a node which is however described as a pivotal table (having geometry field) for all network elements. The simple next statement will lead into orphan elements:

delete from qwat_od.node where id = 116

Furthermore since all is handled by nested triggers it's very difficult to debug and follow what happens in database.

Example of the call graph triggered when we try to insert a new valve (from qgis):

└── insert into vw_element_valve
    └── trigger ft_element_valve_insert
        ├── insert into vw_node_element
        │   └── trigger ft_node_element_insert
        │       ├── insert into network_element
        │       └── fn_node_create
        │           └── insert into node
        └── insert into valve
            └── trigger ft_value_node_set_type
                └── fn_node_set_type
                    ├── delete from node
                    └── update node

Note: There are 68 triggers in the qwat_od schema mainly used to propagate object creation, updating and deleting.

Code organisation

• lack of licence file and contributors list: fixed in #dc3d26
  • still needs a licence for sql code (although a part of it is generated in python)
• fme scripts stands in the repository and seems to be hardcoded to a specific use-case (migration from topobase 2): moved to a specific repo see #71

Maintenance scripts

Bash scripts are used to

- initialize the database model
- make migrations
- insert some views, triggers and functions

• These tools makes qwat's administration specific mostly to unix users

suggestion: using a mix of yaml/ini files with a python script should be by far more flexible and cross-platform. see #86

• In the init_script.sh (https://github.com/qwat/qwat-data-model/blob/a54cea3ce60d0d811f7ac74100cbc53cc887fac9/init_qwat.sh), ~/.pg_service.conf or PGSERVICEFILE environnement variable should not be mandatory, only optional and we should pass database connection directly as parameters to the script.

• In insert_views.sh the sql code is injected and generated with python scripts which prints some sql lines, jump to Python section to see what's under the hood. Example line:

    psql -v ON_ERROR_STOP=1 -c "$(./ordinary_data/views/inheritance/od_installation_inheritance.py ${PGSERVICE})"
  

• cascading calls to sh scripts should be avoided, which will make code easier to maintain and test. Nested calls example : init_qwat.sh -> insert_views.sh -> python scripts.

suggestion: all the sql files should be called from one script and all generated sql code from one other script, no more.

Documentation

• no documentation system: added sphinx documentation coupled with readthedoc for hosting : see #26

Python code

• The python code does not conform to python style guide : https://www.python.org/dev/peps/pep-0008/

  How to to check pep8 on all python files :

  find . -name "*.py" | xargs pep8 --exclude=E501

  This was fixed partially with autopep8 in #4beee5

• The python code is not documented.

• Example of script printing sql code : data-model/metaproject/postgresql/pg_inheritance_view/pg_inheritance_view.py is used to create triggers that simulates inheritance between abstract types and concrete types (node->network_element->elements), sql code is constructed with string concatenation at every levels. The readability is really impacted and performance too (https://wiki.python.org/moin/PythonSpeed/PerformanceTips#String_Concatenation)

  Suggestion: using string formatting or a modern templating engine like jinja2, this should simplify code generation and will be easier for everyone who wants to contribute.