Implement #374 - Support restrictions for GetSchema #616
Conversation
bgrainger
left a comment
bgrainger
left a comment
There was a problem hiding this comment.
Needs tests of the new method.
| close = m_connection.Close; | ||
| } | ||
|
|
||
| var dataTable = m_connection.GetSchema(collectionName, restrictions); |
There was a problem hiding this comment.
I think this will introduce infinite recursion between this method and MySqlConnection.GetSchema.
Please add tests that verify the correct information is being returned, for example like
MySqlConnector/tests/SideBySide/QueryTests.cs
Lines 1147 to 1148 in 5758207
There was a problem hiding this comment.
Yeap! I will add all the tests once I get things right. I am just confused on how to get the restrictions now if this will lead to a infinite recursion. Could you shed some light, please?
There was a problem hiding this comment.
MySqlConnection.GetSchema calls this method:
(Or it will do, once the overload has changed.)
So this method shouldn't call back to MySqlConnection.GetSchema; it should just do the work.
| return dataTable; | ||
| } | ||
|
|
||
| public DataTable GetSchema(string collectionName, string[] restrictions) |
There was a problem hiding this comment.
I don't think introducing a new method should be necessary; string[] restrictions could be added as a new parameter to the existing method.
There was a problem hiding this comment.
Agreed. Can I make the default value null then?
| { | ||
| m_connection.Open(); | ||
| close = m_connection.Close; | ||
| } |
There was a problem hiding this comment.
I think this should just rely on the connection already being open, instead of opening and closing the connection (unless this is required for compatibility with other drivers).
| dataTable.Rows.Add("Owner", "@Owner", "TABLE_SCHEMA", 2); | ||
| dataTable.Rows.Add("Table", "@Name", "TABLE_NAME", 3); | ||
| dataTable.Rows.Add("TableType", "@TableType", "TABLE_TYPE", 4); | ||
| } |
There was a problem hiding this comment.
The Restrictions schema should have four columns: CollectionName, RestrictionName, RestrictionDefault, RestrictionNumber: https://docs.microsoft.com/en-us/dotnet/framework/data/adonet/common-schema-collections#restrictions
This table is filled with the names of restrictions that are supported by other schema tables.
For example, to indicate three restrictions on the Tables schema, you would add these rows:
| CollectionName | RestrictionName | RestrictionDefault | RestrictionNumber |
|---|---|---|---|
| Tables | Schema | 1 | |
| Tables | Table | 2 | |
| Tables | TableType | 3 |
While it's not OK to bring code from Connector/NET into this project, it is totally fine to perform "black box" testing and see what (for example) connection.GetSchema("Restrictions") returns when executed against that library. This code should return a subset of what it supports.
|
To fix the DCO errors, please attest that you agree to the Developer Certificate of Origin (that is, you have the right to submit this code) by adding the line More details here: https://probot.github.io/apps/dco/ |
Signed-off-by: Luiz Eduardo <dudu.christo@gmail.com>
|
Hi @bgrainger. I am still lost on how to proceed with the restrictions array. |
|
@dechristo I would lean towards making this all data-driven. Some ideas:
|
|
@bgrainger Hi, is the restrictions overload actually working now, returning correctly filtered results, or is this a won't-fix? |
|
@KristianWedberg The issue is still open: #374 I'm closing out this three-year-old PR as the author seems to have abandoned it, and it would need some work to fix all the merge conflicts with |
3 participants
No description provided.