Make PyCDS schema name agnostic #44
Conversation
rod-glover
force-pushed
the
i32-2-schema-name-agnostic
branch
from
0fa5f80 to
8c3f0eb
Compare
There was a problem hiding this comment.
Looks straightforward to me and it seems to be a reasonable solution to set the schema at run time with an env var.
One comment: should we advise the user of the specific outcome of setting the schema? Your comment just says to "specify PYCDS_SCHEMA_NAME correctly" without advising what "correct" is. Is it sufficient (and correct) to say that PYCDS_SCHEMA_NAME must agree with the schema location of an existing database? And that for creating a new database PYCDS_SCHEMA_NAME may be set to anything?
One question: You've left logic in (in prefix()) to allow for unspecified schemas, however prefix() is never called with default args and crmp will always be used by default rather than going unspecified. That's not a problem, but was that intentional? Do you see a use case for unspecified schemas or should we continue to have everything fully qualified?
pycds/functions.py
Outdated
| @@ -68,11 +74,12 @@ | |||
| LANGUAGE plpgsql VOLATILE SECURITY DEFINER | |||
| COST 100 | |||
| ROWS 1000; | |||
| ''') | |||
| '''.format(prefix(schema))) | |||
There was a problem hiding this comment.
What? You're not going all-in on f-strings yet?! :p
|
I've addressed both comments (and f-strings!) in the code. Good suggestions. |
pycds/functions.py
Outdated
| RETURNS text AS | ||
| $BODY$ | ||
| stn_query = "SELECT * FROM crmp.getStationVariableTable(" + str(station_id) + ", false)" | ||
| stn_query = "SELECT * FROM getStationVariableTable(" + str(station_id) + ", false)" |
There was a problem hiding this comment.
{schema}.getStationVariableTable
| @@ -29,6 +29,7 @@ | |||
| """ | |||
|
|
|||
| from sqlalchemy.schema import DDL | |||
| from pycds import get_schema_name | |||
There was a problem hiding this comment.
There is a set of bugs, or at least shortcomings, introduced in these function definitions. The assumption we are now (mostly) making is that the search path is not to be relied on, and schema name is explicit everywhere. Except that is not the case here. These definitions were copied direct from the CRMP database definitions and they didn't include explicit schema names.
A couple of examples noted in comments below. The problem occurs wherever a database entity is referred to in a function - another function , a table, view, etc.
|
@jameshiebert , regarding the lack of explicit schema name in function bodies pointed out above: I think I should interpolate schema names throughout. It's not hard to do, although without tests for every function, it would be easy to make an undetected mistake. Your thoughts? |
Sounds good to me.
Yes that's true, though with a careful read, I'd think you should be able to find them all. |
|
@jameshiebert , I had a little feeling that not all was well in all functions. The smoke tests proved out that intuition, and I corrected most of the problems. Definitely worth the effort. Two problems remain: Any further comments? |
|
No further comments from me. |
Resolves #32
This PR replaces the hard-coded schema name
'crmp'throughout PyCDS with a schema name determined by the value of the environment variablePYCDS_SCHEMA_NAME. IfPYCDS_SCHEMA_NAMEis not defined, the schema name defaults to'crmp', making this change fully backward compatible with existing code.To do: