DSN GUI editor #32
DSN GUI editor #32
Conversation
- implement prompt_user_config() based on EsOdbcDsnEdit() entry point.; - implement the callbacks on the GUI actions; - implement DSN bundle serializing to 00-list (which is going to be the way to send/receive the DSN at interfacing point). - start a unit test on the 'dsn' module.
- check for minimum requirements when configuring a DSN (it's name and server location); also perform a DBC configuration with the data, to prevent incomplete data be saved; the connection could still fail later, though, this is just a consistency check with no reachability checks. - perform a connection test in handler when prompting from SQLDriverConnect(), to fail there, rather than later in the function when the error message will not be forwarded back to the user.
- implement prompt_user_config() based on EsOdbcDsnEdit() entry point.; - implement the callbacks on the GUI actions; - implement DSN bundle serializing to 00-list (which is going to be the way to send/receive the DSN at interfacing point). - start a unit test on the 'dsn' module.
- check for minimum requirements when configuring a DSN (it's name and server location); also perform a DBC configuration with the data, to prevent incomplete data be saved; the connection could still fail later, though, this is just a consistency check with no reachability checks. - perform a connection test in handler when prompting from SQLDriverConnect(), to fail there, rather than later in the function when the error message will not be forwarded back to the user.
…-sql-odbc into feature/gui_config
- documentation indicates that last parameter returns number of written bytes, but it's actually characters. Also: - rename 'list00' paramter to 'dsn_str' since the GUI-driver interface will now pass connection strings (since .net framework has a parser for it); - add more DSN validation and fill in the default values for attributs not provided by user.
- Add an editor that is able to edit the DSN as a connection string. - Add it into CMakeLists.txt as a "custom" target, that is also target-aware. - change the build.bat to "recognize" on subsequent invocations the initially set build type.
- add the editor DLLs into the unit tests running dir
…-sql-odbc into feature/gui_config
In count mode (output string is NULL), the function is supposed to return the min size of a buffer that could accomodate the connection string. This commit fixes it's calculation. A unittest was added for it, too.
- place all functionality in one function Also: - fix registry writing for update and new DSN addition.
- bubble up more error messages from DSN validation through DBC configuration; - connection testing and DSN saving callbacks now use the DSN validation function to do most of their work; Also: - add []-framing to IPv6 addresses in URLs - make connection string printing more robust in case destination buffer is too small for the result.
Disable name and description on edit - fixes #28
…-sql-odbc into feature/gui_config
…tname / name. Partially fixes #29 Don't clear the connection string Builder on form close (fixes connection bug)
Mask password box. Refactor StripBraces() method. Added [STAThread] attribute to the factory method.
- now UTF8 - remove BOM
- this fixes the file chooser issue; - DSN form now rendered with .ShowDialog() to prevent calling app allow the user to re-invoke the bridge (which would fail, it running as an STA) fixes #25
…-sql-odbc into feature/gui_config
- in case STA init'ing fails, create a new thread for the form; - also, make the window handler a static member of the binding class. closes #25
… the user that the operation is underway and that they should wait. Fixes #30
…-sql-odbc into feature/gui_config
- this will be used to enable/disable the per-dbc logging - the tick-box in the GUI is now left unchecked if the corresponding parameter is missing from the connection string.
| @@ -140,14 +152,38 @@ namespace EsOdbcDsnBinding { | |||
| } | |||
|
|
|||
| public:int EsOdbcDsnEditor() | |||
| { | |||
| Thread ^ t; | |||
| #ifdef __cplusplus | ||
| extern "C" | ||
| #endif /* __cpluplus */ | ||
| #ifdef _WINDLL |
There was a problem hiding this comment.
Is _WINDLL the correct choice of macro to decide whether the function is being imported or exported?
If this header were included into a file destined for a DLL but NOT the DLL containing the function definition then the function would need to be imported, not exported.
There was a problem hiding this comment.
Sorry for that. I've pushed a fix for it, to use the dedicated DRIVER_BUILD macro (the _WINDLL had stayed there from the PoC time, when I was using a simple app for the form).
- use DRIVER_BUILD macro, which signals a driver build
- add Elastic's license to all source files
|
Some of the C++ files have a |
- rename EsOdbcDsnBinding.cpp to EsOdbcDsnBinding.cc, to be consistent with the rest of the repo (.cc test files)
|
Thanks for pointing that out, renamed the .cpp file. |
|
I don't really know enough about C# to sensibly review those parts. If @codebrain is happy with the C# parts I'm happy to merge the whole PR. |
|
@russcam - I am happy with the C# parts (there isn't a huge amount of code). If you are able to give it a review on (or before!) Monday (22nd October) sometime that would be really appreciated. |
There was a problem hiding this comment.
I've left some general comments.
I see that there are no tests for the configuration editor - will this be covered by integration tests?
I also think that it would be good to consolidate the GUI editor projects with the installer project, under a single solution; This may also make it easier to automate the complete build
| return true; | ||
| } | ||
|
|
||
| MessageBox.Show("Log directory invalid", "Error", MessageBoxButtons.OK, MessageBoxIcon.Error); |
There was a problem hiding this comment.
Should indicate why the directory is invalid
There was a problem hiding this comment.
Error now reads: Log directory invalid, path does not exist
| private bool ValidateCertificateFile(string file) | ||
| { | ||
| if (string.IsNullOrEmpty(file) | ||
| || (File.Exists(file) && File.ReadAllBytes(file).Length > 0)) |
There was a problem hiding this comment.
Maybe
new FileInfo(file).Lengthinstead of
File.ReadAllBytes(file).LengthCan also use the FileInfo for the .Exists call then too.
Should the certificate be validated here as well?
There was a problem hiding this comment.
++ to the fileinfo change.
Currently not implementing the certificate validation - at the moment as long as the file exists and is > 0 bytes then we'll let Elasticsearch handle any errors.
| { | ||
| Application.EnableVisualStyles(); | ||
| DsnEditorForm form = new DsnEditorForm(onConnect, dsnIn, delegConnectionTest, delegSaveDsn); | ||
| form.ShowDialog(); // Instead of Application.Run(form); |
There was a problem hiding this comment.
this could probably do with a comment for why .ShowDialog() is used instead of Application.Run(...). I can guess, but I think a one line comment may help
There was a problem hiding this comment.
I've added a short explanation on it.
| // General Information about an assembly is controlled through the following | ||
| // set of attributes. Change these attribute values to modify the information | ||
| // associated with an assembly. | ||
| [assembly: AssemblyTitle("Elasticserach DSN Editor")] |
| [assembly: AssemblyTitle("EsOdbcDsnEditorLauncher")] | ||
| [assembly: AssemblyDescription("")] | ||
| [assembly: AssemblyConfiguration("")] | ||
| [assembly: AssemblyCompany("")] |
- add an explanation as to why the editor form is rendered with .ShowDialog() instead of Application.Run().
|
@russcam - There are no tests for the editor yet. I was in two minds about how to acheive this, either via UI automation tests or moving to MVVM / MVC in Winforms (not great...). I think for alpha its OK to roll without tests - something to be addressed next. |
|
Just had a look over the changes, and they LGTM. IMO, it would be good to have some tests, even for alpha. Is it also worth including some logging for the UI, for users to be able to provide in the event of exceptions? |
@russcam It would be indeed good to add tests and testing will be my goal for the rest of the remaining weeks till releasing.
That's an interesting thought. I'll work with @codebrain to see what we could do here. |
This PR adds a .NET DSN editor.
The editor is being invoked in two circumstances:
The editor lives now under
dsneditorfolder and brings two more DLLs to the driver:EsOdbcDsnEditorsubfolder, a C# project);EsOdbcDsnBindingsubfolder, a C++/CLI project).The interface between the driver and the editor consists of a main function
EsOdbcDsnEdit()(defined in the binding project). The function takes as parameters (among others):The driver does the validation of the parameters and returns any error message back through callbacks' output parameters.