Stop emitting REVOKE statements when yamltodb was run with --no-privileges

[benkuhn]

Hi, thanks for making an awesome tool!

I'm using pyrseas to auto-generate skeleton db migrations, running dbtoyaml with --no-privileges. It would be really helpful if I could stop it from emitting statements like REVOKE ALL ON SCHEMA public FROM PUBLIC. Test case:

$ python pyrseas/dbtoyaml.py --no-owner --no-privileges test | tee db.yaml
extension plpgsql:
  description: PL/pgSQL procedural language
  schema: pg_catalog
  version: '1.0'
schema public:
  description: standard public schema
$ python pyrseas/yamltodb.py test < db.yaml
REVOKE ALL ON SCHEMA public FROM PUBLIC;

REVOKE ALL ON SCHEMA public FROM ben;

Based on skimming the source, it seems like this might be fixed by adding a check in dbobject.DbObject.diff_map not to emit REVOKE lines if inobj.privileges is None, the same way it works for owner. Is that accurate? I'm happy to submit a patch if so. (I definitely don't understand the full context here, so please correct me if I'm wrong!)

[jmafc]

Sorry for the delay in responding.

The REVOKE's are emitted by add_revoke in dbobject/privileges.py which is being called from two places in diff_privs (in the same file) which in turn is called by DbObject.diff_privileges and in turn by DbObject.diff_map. Are you suggesting adding a conditional inobj.privileges is not None in front of the call to diff_privileges in diff_map?

I haven't looked at this for a while, but intuitively it doesn't seem like it would do the trick, but I may be wrong. I'll have to take a closer look in the coming week.

[benkuhn]

Ah--looks like inobj.privileges is the empty list, not None, if the YAML file doesn't define privileges. So adding the is not None check doesn't work, but it does pass my silly test case above if you instead check the objects for truthiness (I made #154 to demonstrate).

Seems like the particular change I made in the PR breaks other expected behavior (per the CI failures), but is there an alternative that would work better? Perhaps altering the YAML parsing to differentiate between an empty list of privileges and a null value and then doing the null-check approach?

[jmafc]

This problem has been there for a very long time (I probably ought to test on some old version(s) to verify how far back and then use git bisect to attempt to figure out what may have caused it). I think it may be due to lack of symmetry between dbtoyaml (which has a -x option) and yamltodb (which doesn't) and/or possibly the special handling of the public schema.

[benkuhn]

Cool. If you're bisecting, here's the test case script I was using:

#!/bin/bash
DB_NAME=__priv_test__
createdb $DB_NAME &>/dev/null
python pyrseas/dbtoyaml.py --no-owner --no-privileges $DB_NAME | tee db.yaml
echo '=== SQL output ==='
python pyrseas/yamltodb.py $DB_NAME < db.yaml | grep REVOKE
if [ "$?" -eq 0 ]; then exit 1; else exit 0; fi

I couldn't get it to run on earlier than v0.6.0 and the issue was present then. (On v0.5.0 I got the following traceback while running the script):

Traceback (most recent call last):
  File "pyrseas/yamltodb.py", line 70, in <module>
    main()
  File "pyrseas/yamltodb.py", line 45, in main
    stmts = db.diff_map(inmap, args.schlist)
  File "/Users/ben/code/Pyrseas/pyrseas/database.py", line 200, in diff_map
    self.from_map(input_map)
  File "/Users/ben/code/Pyrseas/pyrseas/database.py", line 152, in from_map
    for key in input_map.keys():
AttributeError: 'NoneType' object has no attribute 'keys'

[jmafc]

The error in v0.5.0 yamltodb is because (I'm assuming) you used another db.yaml. If you run v0.5.0 dbtoyaml --no-owner --no-privileges, you would've seen an earlier error:

usage: dbtoyaml.py [-h] [-H HOST] [-p PORT] [-U USERNAME] [-W] [-o OUTPUT]
                   [--version] [-n SCHEMA] [-t TABLIST]
                   dbname
dbtoyaml.py: error: unrecognized arguments: --no-owner --no-privileges

IOW, the REVOKE problem has existed from the very beginning of adding privilege processing to Pyrseas.

[jmafc]

As predicted, git bisect reports:

4b7d2d7660633465a4067e006e7dfad1579e469b is the first bad commit
commit 4b7d2d7660633465a4067e006e7dfad1579e469b
Author: Joe Abbate <[email protected]>
Date:   Wed Aug 15 16:01:55 2012 -0400

    Add support for changing privileges on tables, functions and other objects.

[jmafc]

So here's the story and it goes back to the asymmetry in handling owners/privileges between dbtoyaml and yamltodb. If dbtoyaml is run without options on an "empty" database, the typical ouput is:

extension plpgsql:
  description: PL/pgSQL procedural language
  owner: postgres
  schema: pg_catalog
  version: '1.0'
schema public:
  description: standard public schema
  owner: postgres
  privileges:
  - postgres:
    - all
  - PUBLIC:
    - all

When processing the public schema in diff_privs , we compare currprivs to newrprivs and find them to be equal so no REVOKEs are emitted.

OTOH, if we run dbtoyaml --no-owner --no-privileges the YAML output is of course lacking the owner and privileges lines/blocks. When diff_privs processes the public schema it finds that currprivs are empty so it calls add_revoke to drop the privileges.

I'm not sure how to fix it. One option would be to assume that if currprivs is empty and newprivs are ALL to PUBLIC and postgres, we don't need to call add_revoke but that looks like a special case hack.

To address this in a more generalized manner, we ought to close the asymmetry, i.e., add --no-privileges and --no-owner options to yamltodb. I think at one point I was going to go down that road, but I don't recall whether I didn't because of time, distractions or some technical issue. A possible alternative would be to have the from_map methods keep track whether any owner or privileges attributes were found in any object in the input YAML. If none were seen, then presumably the map was created with --no-owner --no-privileges so we should skip owner/privilege processing accordingly in yamltodb.

[benkuhn]

Yeah, the special case hack wouldn't solve things for me or (I think) other Mac users, because the default Postgres user is $USER and not postgres.

It looks like the from_map methods already keep track of this for owner (I'm basing this on the inobj.owner is not None check here which looks like it's meant to trigger in the case when there's no owner key in the YAML?)--could a parallel thing be done for privileges?

[jmafc]

That no_owner argument to diff_map was added to deal with #88 because PG doesn't allow ALTER EXTENSION to change the owner. My point is that I don't think we've generalized the handling of owner attributes in yamltodb.

What I think we need to do is (a) determine if we can implement a process to keep track of whether the input map has owner/privilege information--without much ugliness such as globals or passing extra arguments all over the place, and (b) decide what needs to take place when yamltodb faces a --no-privileges request (either as an command line option or as determined by (a)). Perhaps we should start with adding some tentative new test cases in tests/dbobject/test_privs.py which parallel the existing ones.

[jmafc]

@benkuhn Please read https://pyrseas.wordpress.com/2018/09/12/the-future-of-pyrseas-revisited/ .