Thread: PATCH: Column Level Privileges
Hi All,
Please find the patch for Column Level Privileges support.
I have also attached some of the test-cases for it.
Please find the patch for Column Level Privileges support.
I have also attached some of the test-cases for it.
--
On Thu, Jan 29, 2009 at 5:17 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote: > Hi All, > > Please find the patch for Column Level Privileges support. > I have also attached some of the test-cases for it. Hi Ashesh, Couple of minor issues/questions: - Why didn't you derive the dialog from dlgSecurityProperty? I assume it would have been too messy as it's already derived from dlgTypeProperty? - As a general rule, we disable controls not relevant to a particular version of Postgres. We should do the same with this tab - disable the controls rather than hide them on servers < 8.4. - Can we lose the comment above the column permissions in the reverse engineered SQL please? We don't add similar ones for other additional queries so we shouldn't here (though perhaps in the future we might add such comments everywhere that is appropriate). - Not just an issue with your code, but also the existing privilege tabs - could you please tweak the column sizes of the list control such that the headers can be read by default? Thanks! -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Hi Dave,
If we derive the dlgColumn from both of them, it will leads to the famous Diamond problem.
We can avoid this problem by multiple inheritance using virtual base class.
And hence, it will increase the complexity. :(
dlgSecurityProperty and dlgTypeProperty both are derived from dlgPropery.Couple of minor issues/questions: - Why didn't you derive the dialog from dlgSecurityProperty? I assume it would have been too messy as it's already derived from dlgTypeProperty?
If we derive the dlgColumn from both of them, it will leads to the famous Diamond problem.
We can avoid this problem by multiple inheritance using virtual base class.
And hence, it will increase the complexity. :(
sure.- As a general rule, we disable controls not relevant to a particular version of Postgres. We should do the same with this tab - disable the controls rather than hide them on servers < 8.4.
Sure.- Can we lose the comment above the column permissions in the reverse engineered SQL please? We don't add similar ones for other additional queries so we shouldn't here (though perhaps in the future we might add such comments everywhere that is appropriate).
sure.- Not just an issue with your code, but also the existing privilege tabs - could you please tweak the column sizes of the list control such that the headers can be read by default?
I will update you as soon as possible.Thanks!
--
Hi Dave,
Dave Page wrote:
Changed the column size to 70.
I think - it should fit in all the platform.
Please find the updated patch.
Dave Page wrote:
Done- As a general rule, we disable controls not relevant to a particular version of Postgres. We should do the same with this tab - disable the controls rather than hide them on servers < 8.4.
Done.- Can we lose the comment above the column permissions in the reverse engineered SQL please? We don't add similar ones for other additional queries so we shouldn't here (though perhaps in the future we might add such comments everywhere that is appropriate).
Done.- Not just an issue with your code, but also the existing privilege tabs - could you please tweak the column sizes of the list control such that the headers can be read by default?
Changed the column size to 70.
I think - it should fit in all the platform.
Please find the updated patch.
--
Hi Ashesh On Fri, Jan 30, 2009 at 6:48 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote: > Please find the updated patch. Thanks - unfortunately, I found a couple of problems. - If the dialog is the default (minimum) size when opened, the privileges pane does not size correctly. Observed on Windows. - If I open the table dialogue on an existing table, and then select a column and click change. and then change an existing ACL entry, the generated SQL includes a REVOKE ALL which revokes privileges from other columns on the table. /D -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Hi Dave,
Dave Page wrote:
Could you please send a screen shot for it?
There was one problem with GRANT ALL too.
And also found one problem, when privileges are for group, "group " was not
appended in privileges lists (ctlSecurityPanel). I found the same problem on
existing privileges dialogs too. :(
I have attached a screen for it. (dlgTable)
In this case, 'ash' is a group. We were not identify the groups from privileges.
We were able to select multiple privileges for 'group ash' and 'ash' both. :(
I have solved that also.
Please find the updated patch.
Dave Page wrote:
I could not reproduce it on windows (Vista). :(- If the dialog is the default (minimum) size when opened, the privileges pane does not size correctly. Observed on Windows.
Could you please send a screen shot for it?
Done.- If I open the table dialogue on an existing table, and then select a column and click change. and then change an existing ACL entry, the generated SQL includes a REVOKE ALL which revokes privileges from other columns on the table.
There was one problem with GRANT ALL too.
And also found one problem, when privileges are for group, "group " was not
appended in privileges lists (ctlSecurityPanel). I found the same problem on
existing privileges dialogs too. :(
I have attached a screen for it. (dlgTable)
In this case, 'ash' is a group. We were not identify the groups from privileges.
We were able to select multiple privileges for 'group ash' and 'ash' both. :(
I have solved that also.
Please find the updated patch.
--
Attachment
Hi On Mon, Feb 2, 2009 at 12:55 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote: > Hi Dave, > > Dave Page wrote: > > - If the dialog is the default (minimum) size when opened, the > privileges pane does not size correctly. Observed on Windows. > > I could not reproduce it on windows (Vista). :( > Could you please send a screen shot for it? Attached before and after resize screenshots (it fixes itself if you resize the dialogue) > And also found one problem, when privileges are for group, "group " was not > appended in privileges lists (ctlSecurityPanel). I found the same problem on > existing privileges dialogs too. :( Thats intentional - we dropped the 'group' keyword when roles were introduced. It looks like your patch fixes a related issue with the incorrect icon being used - we should keep that part of the fix :-) -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Attachment
Hi Dave,
Please find the updated patch.
Dave Page wrote:
As I am not reproduce the same on my Vista, I can not verify it. :(
But, I think - this should resolve the problem.
Please find the updated patch.
Dave Page wrote:
Please check, if this has solved the size problem.Hi On Mon, Feb 2, 2009 at 12:55 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:Hi Dave, Dave Page wrote: - If the dialog is the default (minimum) size when opened, the privileges pane does not size correctly. Observed on Windows. I could not reproduce it on windows (Vista). :( Could you please send a screen shot for it?Attached before and after resize screenshots (it fixes itself if you resize the dialogue)
As I am not reproduce the same on my Vista, I can not verify it. :(
But, I think - this should resolve the problem.
--
On Tue, Feb 3, 2009 at 10:04 AM, Ashesh D Vashi <ashesh.vashi@enterprisedb.com> wrote: > Please check, if this has solved the size problem. > As I am not reproduce the same on my Vista, I can not verify it. :( > But, I think - this should resolve the problem. Nope, doesn't help. I'm surprised you cannot reproduce it on Vista though - what version of wxWidgets are you using (I have 2.8.9)? Note that the size is only incorrect if the dialog opens at the minimum size - if it's any larger, it'll size correctly. You can open the dialogue, shrink it as far it'll go, then close and re-open to see the bug. Guillaume; is this the issue you were adding a block of code to add and then subtract a pixel from the height and width to fix? -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Tue, Feb 3, 2009 at 10:04 AM, Ashesh D Vashi > <ashesh.vashi@enterprisedb.com> wrote: > >> Please check, if this has solved the size problem. >> As I am not reproduce the same on my Vista, I can not verify it. :( >> But, I think - this should resolve the problem. > > Nope, doesn't help. I'm surprised you cannot reproduce it on Vista > though - what version of wxWidgets are you using (I have 2.8.9)? Note > that the size is only incorrect if the dialog opens at the minimum > size - if it's any larger, it'll size correctly. You can open the > dialogue, shrink it as far it'll go, then close and re-open to see the > bug. > The way to reproduce it is this one: * open the dialog * resize it to its minimum size * close it * reopen it And the UI issue should appear. > Guillaume; is this the issue you were adding a block of code to add > and then subtract a pixel from the height and width to fix? > Yes, that's right. If that's the only thing that prevents this patch from being commited, commit it and I'll take a look at the UI glitch. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
On Tue, Feb 3, 2009 at 4:14 PM, Guillaume Lelarge <guillaume@lelarge.info> wrote: > Yes, that's right. If that's the only thing that prevents this patch > from being commited, commit it and I'll take a look at the UI glitch. Thanks - committed. And thanks to Ashesh for the patch! -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page a écrit : > On Tue, Feb 3, 2009 at 4:14 PM, Guillaume Lelarge > <guillaume@lelarge.info> wrote: > >> Yes, that's right. If that's the only thing that prevents this patch >> from being commited, commit it and I'll take a look at the UI glitch. > > Thanks - committed. > I don't find the UI issue. Did you already fix it? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com