Sanitize queries too #185
Sanitize queries too #185
Conversation
|
|
||
| Set qry = dbs.QueryDefs(m_Query.Name) | ||
|
|
||
| If qry.Connect <> vbNullString Then SanitizeConnectionString qry.Connect |
There was a problem hiding this comment.
The main issue here is that if possible, we really don't want to modify the original objects during an export. We certainly wouldn't want to leave them in a modified state, as the purpose of the add-in is non-destructive export of source code, not changing the original database objects.
If a database object is modified, such as changing the .Connect string, I believe it will (obviously) also affect the last modified date of the object. The means the user no longer knows when that query was last modified in the designer. Effectively this loses a piece of information that they had about this particular object.
The above approach is clean and simple, which I like, but it also would change every pass-through query object, regardless of whether or not the connection string needed to be modified. A better approach would be to compare a sanitized connection string with the existing value to see if we actually need to make the change, and only update the ones that actually require the change, then restore the original value after export, but before updating the index.
But probably the best approach would be to parse the export file itself and make our adjustments there. This would be completely non-desctructive to the source database object, faster in performance, and provide a good template for other similar kinds of changes we might need to make in multi-line source file output properties. (Basically we would parse out the full value, make our changes, then add back in the line breaks to match the export file format.)
I do like the idea of updating the connection strings for the pass-through queries, but in my personal experience, linked table connection strings are recreated in the context of the current user, while pass-through query connection strings are not. This means that when two users build the same database, the table connection strings will be different, but the query connection strings will stay the same. (They would retain the connection string originally used in the object definition.)
We solved the table connection strings issue in #181 and I have not had any problem with another user and myself building the same database from source (and exporting source) even though it contains a large number of pass-through queries. This kind of brings the question of whether this is really necessary for queries, since we don't encounter the same issue we had with tables... 🤔 Have you encountered any examples where the exported source output was different for different users based on pass-through query connection strings?
There was a problem hiding this comment.
I agree with not changing the query; wasn't sure I was ready to build another parse tool for the exported query, but since we may need to for the #165, perhaps this is a moot point.
We actually do experience this, as a side-effect of the tabledef in #181. Because we switch between production and test, and a secondary server for fail-back, several of our dynamically spun up pass-through queries grab the connection string from a linked keytable and stick it in; this has the effect of resetting the connection string for each different programmer.
We also would like to sanitize this from queries as much as possible, as it poses a security risk to our environment, and would reduce the need to protect the source code for this particular reason. We do not have any non-trusted connections in our environment (by security design), and any exposed userIDs effectively become a vulnerability.
With that in mind, one approach we may consider is if we create a .json or include the connection string in the .SQL file; and sanitize it from the query export.
There was a problem hiding this comment.
We actually do experience this, as a side-effect of the
tabledefin #181. Because we switch between production and test, and a secondary server for fail-back, several of our dynamically spun up pass-through queries grab the connection string from a linked keytable and stick it in; this has the effect of resetting the connection string for each different programmer.
Thanks for the additional feedback on this! Yesterday I achieved a significant milestone in our environment where a very complex database can be completely rebuilt and exported on two different developer profiles and produce exactly the same source code. This was huge for us, and will really streamline our development cycle on larger projects with multiple developers. I know you will enjoy that feeling too, and it sounds like you are not too far off... 😄
I have some ideas on how to modify the main sanitize function to handle multi-line values, but I don't have the time to fully flesh them out right now... But I did go ahead and write some quick untested conceptual code that you can finish out if you like... (See commit a683747) Hope that helps!
There was a problem hiding this comment.
I'll take a crack at this, I'm similarly under a deluge of other projects that have all become imminently needed :) I'll probably finish out the updated regex parsing, and pick at the commit in a bit.
There was a problem hiding this comment.
Sounds good. Thanks for your work!!
There was a problem hiding this comment.
As I write this, it may be better to write a new class; the tables, queries, passthrough queries, tables, etc. seem to use the same/similar data storage, and in fact, the queries and table exported use the internal Access data types for their exported file definitions:
For example, a query's SQL is actually a dbMemo (text) type field:
`dbMemo "SQL" ="SELECT SOME, COOL, SQL, DATA FROM tbl_myNeatTables;"
the returnsreccords is actually a dbBoolean (BIT) type field:
dbBoolean "ReturnsRecords" ="-1"
And so on.
So...if we do build a parser, perhaps it should be a reusable class (or module? I'm on fence, both have their advantages in this case), instead. This could/(should?) incorporate the connection and the SQL, and therefore be something like the prtdev and clsDevMode that rebuilds the devmode prior to importing the Form/report.
All this to say: what the heck do we name it?
I propose clsSQLGrabberator. Alternately, and more seriously: clsSQLDataTools
|
|
||
| ' Check database path to convert to relative | ||
| Case StartsWith(strPart, "DATABASE=", vbTextCompare) | ||
| .Add GetRelativeConnect(strPart) |
There was a problem hiding this comment.
We also need to make sure this still happens... Linked text files, (such as the one in Testing.accdb) include a path that should be converted to relative.
There was a problem hiding this comment.
This is pretty straightforward; I'll toss this in, quick like.
There was a problem hiding this comment.
I lied; this actually led me down a rabbit hole I have to abandon for the moment.
I have some thoughts on how better to handle this, but a few other jobs are going to monopolize my time for a bit.
As a test case: the Database should never be a relative path if Server is set, correct?
(cherry picked from commit d4904cd61a53a70321f750ccf21e7ac1cd16ec0f)
(cherry picked from commit d7f8bae20ab5a9f9fb7aa27326b55dbdd84bd881)
The Sanitize function should either return the original string, or a modified version of the original string. Added some additional comments as well.
These can be run using the RubberDuck testing framework.
This is some UNTESTED and UNFINISHED conceptual code to illustrate an approach on parsing out the wrapped connection string line from an export file. I am hoping it will be a help to @hecon5 in building out this functionality.
modified: Version Control.accda.src/modules/clsDbTableDef.bas new file: Version Control.accda.src/modules/clsSQLDataTools.bas Placed changes from clsDbTableDef onto clsSQLDataTools
… Also adding in a test where the server/database are not in the usual order, and adding other options to the end.
hecon5
force-pushed
the
sanitizeQueriesToo
branch
from
c0895ca to
1d7bb79
Compare
|
I'm going to close this PR for now; I can't finish it in the next few weeks due to other time commitments; but I'll keep these notes for now. |
Incorporate sanitize in queries, as well.