Add ObjectName class for representing myschema.myobject references #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This includes getting the following functions to all work with DBObjects: * determine_nonschema_privileges_for_schema * collapse_personal_schemas * add_privileges
Doing schema."table" makes sense for real objects, but * isn't a real object. As a result, doing foo."*" is confusing: it seems like we're talking about a real object name '*', but we're not. To avoid this confusion, we output foo."bar" for everything except when the object_name is a *, in which case we do foo.*
DBObject is used in many spots, so it makes sense to expose this in common.py Also, originally this was two commits: * move DBObject to common.py * Rename DBObject to ObjectName However, during an interactive rebase I accidentally squashed this into one commit and am gonna just leave it that way.
ObjectName.object_name is confusing (it should be redundant but isn't: we're referring to the object name without a schema). Unfortunately, Postgres refers to entities as "objects", so we don't want to just called things ObjectName.object either. As a result, the clearest name is probably just ObjectName.unqualified_name, so we switch to that.
When a test fails it is useful to see what the ObjectName instance represents. Having a repr facilitates that.
It is better practice for us to amend our custom Dumper class rather than amend the default one, which may introduce hard-to-trace issues in other tests or downstream in the codebase.
All lists within the owns and privileges subdicts will be converted to
ObjectNames. If there are any empty entries, e.g.:
roleA:
owns:
tables:
...
Then these empty entries get converted to None by PyYAML. Rather than
override this, I'm leaving this behavior as:
1) It adds a some complexity to try to guard against Nones in a
bunch of spots.
2) More than the complexity, it makes the code less readable
3) And most importantly, the point of pgbedrock is to make a spec
simply to read and follow. Those empty entries are unnecessary
cruft.
As a result, it is both easier on our end and more correct based on this
tool's philosophical purpose to not try to convert entries from None to
an empty list.
Originally, I had tried to use a customer PyYAML constructor to
automatically convert these sublists from strings to ObjectName
instances, but I found PyYAML confusing and complicated enough that
whoever would have to maintain this code if it breaks would probably end
up quite confused. Because of that it seemed wiser to just let PyYAML do
its thing and then we come in with regular Python and convert the
relevant parts of the loaded spec into ObjectName instances.
ensure_quoted_identifier() was necessary before we had ObjectName instances. Now it is unused and can be removed.
This shouldn't make a difference but is technically more correct. In addition, it is a good way to verify that we are indeed passing in ObjectName instances that represent schemas and not non-schema objects.
zcmarine
changed the title
1 similar comment
cpdean
approved these changes
Jul 2, 2018
There was a problem hiding this comment.
This is quite large.
It might have been better to try to have a transition period between the two internal API's (string based vs objectname based), but I guess sometimes you gotta pull that bandaid off fast.
I've traced a few codepaths and nothing seems wrong -- wherever there used to be a string there is now an ObjectName. I thought I almost caught a few things you missed, but I was wrong. Like -- you started using ObjectNames as keys in a dict, which my reflex is that this would be a bug, but I see you implemented __hash__ so that's fine, etc.
Some day we'll have static types and big PR's like this will feel trivial ❤️.
Glad to see how far this project has come.
Best,
Conrad
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the worst PR I've ever made. To try to justify this ugly mess so I don't feel so lousy: the choice was between one huge, incomprehensible PR (this) and 48 tiny PRs that were comprehensible and would leave the repo in a good place. My reasons for going this route were:
With the above given as an attempt to exonerate myself from guilt (which I still feel), I think it'd be useful to discuss this PR in person and see if there's anything else that would be useful to do for due diligence. Actually reviewing the medley of changes seems like a bad use of time.