DASSL / ClassDB

An open-source system to let students experiment with relational data
https://dassl.github.io/ClassDB/
Other
7 stars 2 forks source link

Missing checks if user/schema already exists (M) #136

Closed smurthys closed 6 years ago

smurthys commented 6 years ago

Function createUser in addUserMgmt.sql does not seem to handle a few issues if a user/schema already exists

This issue is created based on the discussion of Issue #1.

afig commented 6 years ago

Raising an notice/exception would be the best course of action. Given that other parts of ClassDB depend on there being a correspondence between user and schema names, I believe that an exception would be best, so that there doesn't exist the possibility of there being a user without a corresponding schema they have access to. At minimum, a warning should be raised

It might be helpful to view the table of PostgreSQL's message severity levels, copied here:

Severity Usage syslog eventlog
DEBUG1..DEBUG5 Provides successively-more-detailed information for use by developers. DEBUG INFORMATION
INFO Provides information implicitly requested by the user, e.g., output from VACUUM VERBOSE. INFO INFORMATION
NOTICE Provides information that might be helpful to users, e.g., notice of truncation of long identifiers. NOTICE INFORMATION
WARNING Provides warnings of likely problems, e.g., COMMIT outside a transaction block. NOTICE WARNING
ERROR Reports an error that caused the current command to abort. WARNING ERROR
LOG Reports information of interest to administrators, e.g., checkpoint activity. INFO INFORMATION
FATAL Reports an error that caused the current session to abort. ERR ERROR
PANIC Reports an error that caused all database sessions to abort. CRIT ERROR

A message with a severity of ERROR would be raised if we decide to go with an exception.

smurthys commented 6 years ago

I propose starting a Wiki page with a flow chart showing the conditions and actions. Though it might sound like a lot of work, in all my experience, this approach will likely take less time overall: reduces number of code iterations.

The chart can be made part of the docs when the issue is addressed.

afig commented 6 years ago

I'll get started on creating a flow chart showing the actions taken based on whether a user and/or schema already exist for the role being created. I'll update this comment once that flow chart is available on the Wiki.

[A flowchart and a description](https://github.com/DASSL/ClassDB/wiki/createRole()-User-Schema-checks-and-actions) of the proposed checks, actions, and messages is now available on the Wiki.

smurthys commented 6 years ago

Thanks @afig for the flowchart. I will make sure my implementation indeed follows this design. (I think it does). I will also update if I find any improvements possible.

FYI, I have created a related Issue #141 on the docs-development process.

afig commented 6 years ago

I have reviewed the checks performed by createRole and everything that is necessary and mentioned in the diagram is performed. So in terms of the original intent of this issue, it can be closed.

However, one new issue is that the flowchart does not show the correct sequence of the checks performed, and that the createRole function now performs additional checks that are not shown on the flowchart. Also, the messages output by the function are different from what is on the flowchart (which was expected).

In order for it to be a useful documentation and troubleshooting tool, the flowchart should closely, if not exactly, match the behavior of the function. However, at this point this is more of a documentation issue. I propose closing this issue, since the original issue has been solved. I have added a list item to the Documentation section of the M2 checklist in regards to updating the flowchart.

afig commented 6 years ago

[The flowchart](https://github.com/DASSL/ClassDB/wiki/createRole()-User-Schema-checks-and-actions) has been updated to match latest system behavior; as this was the last remaining requirement for this issue, I am closing it.

smurthys commented 6 years ago

The flowchart is fantastic. Thanks @afig for creating it.

I think it needs an attribution box with you, steve, and myself mentioned.

afig commented 6 years ago

Thanks @smurthys. I've added an attribution box to the diagram and fixed a couple of small issues I noticed.

smurthys commented 6 years ago

Looks good @afig.

Would you please add the HTML and a full-size image (PNG) to docs? Also add a "small" image, but I am not sure what the scale down should be given the diagram complexity. Please choose something reasonable with the knowledge it won't be very readable. We will provide the user the option of seeing the full-size image.