Bug Hunting Session
Bug 45775 - PgSQL new DB allow empty Datasource
Summary: PgSQL new DB allow empty Datasource
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
3.5.0 release
Hardware: All All
: medium minor
Assignee: Akshay Deep
URL:
Whiteboard: target:5.2.0
Keywords: difficultyBeginner, easyHack, skillCpp, skillVcl, topicUI
Depends on:
Blocks:
 
Reported: 2012-02-08 05:52 UTC by Lionel Elie Mamane
Modified: 2016-10-25 19:09 UTC (History)
9 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2012-02-08 05:52:43 UTC
With PostgrSQL, an empty Datasource URL is perfectly valid (connect to localhost / UNIX socket).

The "new database wizard" (choose "connect to an existing database", PostgreSQL and "next") requires something, even if only a space character. Change it to allow  empty.

It happens in source/ui/dlg/ConnectionPage{Setup,}.cxx
Comment 1 sasha.libreoffice 2012-05-16 02:37:40 UTC
Easy hack?
Comment 2 Jochen 2012-08-25 20:13:23 UTC
Is your "observation" still applicable for LO 3.6.1 RC1?
Comment 3 Anderson Cordeiro 2013-05-04 12:59:33 UTC
Hello friends
I'm working on it!

hugs
Comment 4 Lionel Elie Mamane 2013-09-02 12:08:37 UTC
Anderson, are you still working on it? Any chance to see a patch arise from your work? I would gladly review it.
Comment 5 Joel Madero 2014-02-27 23:14:32 UTC
In order to limit the confusion between ProposedEasyHack and EasyHack and to make queries much easier we are changing ProposedEasyHack to NeedsDevEval.

Thank you and apologies for the noise
Comment 6 Jens Carl 2014-06-04 00:57:22 UTC
Hey,

I would like to take this bug, but I some questions before I can start:

Is it still valid to have an empty string for the connection?
The dlg now resides dbaccess/source/ui/dlg?

The described behaviour still occurs in current master.

Jens
Comment 7 Lionel Elie Mamane 2014-06-18 17:28:49 UTC
Terribly sorry it took me so long to respond.

(In reply to comment #6)

> I would like to take this bug, but I some questions before I can start:

> Is it still valid to have an empty string for the connection?

Yes. That's a PostgreSQL feature.

> The dlg now resides dbaccess/source/ui/dlg?

Yes, AFAIK the code handling that dialog is still there. The dialog itself is the .src/.hrc file, and the .cxx files contain the C++ code that implements the actual actions behind the dialog.

> The described behaviour still occurs in current master.

Yes, as can trivially be checked by "New Database" / Connect to an existing / PostgreSQL / Next, notice that the "Next" field is greyed out until one enters something in "Datasource URL".
Comment 8 Alex Thurgood 2015-01-03 17:39:42 UTC
Adding self to CC if not already on
Comment 9 Lionel Elie Mamane 2015-05-01 05:46:16 UTC
The dialog has now been migrated to the new ".ui" system. The dialog itself now resides in dbaccess/uiconfig/ui/connectionpage.ui

The code that handles it is still in dbaccess/source/ui/dlg/ConnectionPage{Setup,}.cxx which also uses dbaccess/source/ui/dlg/ConnectionHelper.cxx
Comment 10 Lionel Elie Mamane 2015-05-01 06:04:38 UTC
The previous link are about this specific tab page. The whole "tabbed dialog" itself seems to be in:

 dbadmin.cxx
 dbwiz.cxx
 dbwizsetup.cxx

In particular this code looks interesting in dbwizsetup.cxx:

IMPL_LINK(ODbTypeWizDialogSetup, ImplModifiedHdl, OGenericAdministrationPage*, _pConnectionPageSetup)
{
    m_bIsConnectable = _pConnectionPageSetup->GetRoadmapStateValue( );
    enableState(PAGE_DBSETUPWIZARD_FINAL, m_bIsConnectable);
    enableState(PAGE_DBSETUPWIZARD_AUTHENTIFICATION, m_bIsConnectable);
    if (getCurrentState() == PAGE_DBSETUPWIZARD_FINAL)
        enableButtons( WZB_FINISH, true);
    else
        enableButtons( WZB_FINISH, m_bIsConnectable);
    enableButtons( WZB_NEXT, m_bIsConnectable  && (getCurrentState() != PAGE_DBSETUPWIZARD_FINAL));
    return sal_True;
}


So we need to look at how m_bIsConnectable is set from GetRoadmapStateValue( ).

GetRoadmapStateValue( ) is only

dminpages.hxx:        bool                GetRoadmapStateValue() const { return m_abEnableRoadmap; }

which is set in

adminpages.hxx:        void                SetRoadmapStateValue( bool _bDoEnable ) { m_abEnableRoadmap = _bDoEnable;


So we need to chase where SetRoadmapStateValue is called.


Here are the IMO interesting bits:

* ConnectionPageSetup.cxx:

    bool OConnectionTabPageSetup::checkTestConnection()
    {
        return !m_pConnectionURL->IsVisible() || !m_pConnectionURL->GetTextNoPrefix().isEmpty();
    }

    IMPL_LINK(OConnectionTabPageSetup, OnEditModified, Edit*, /*_pEdit*/)
    {
        SetRoadmapStateValue(checkTestConnection());
        callModifiedHdl();
        return 0L;
    }


Just remove the emptiness test in checkTestConnection()? Try that.
Comment 11 Jens Carl 2015-05-08 15:32:11 UTC
> * ConnectionPageSetup.cxx:
> 
>     bool OConnectionTabPageSetup::checkTestConnection()
>     {
>         return !m_pConnectionURL->IsVisible() ||
> !m_pConnectionURL->GetTextNoPrefix().isEmpty();
>     }
 
> Just remove the emptiness test in checkTestConnection()? Try that.

I did that and it's the right location, but I encountered two problems with that:

1. There is a side effect, that also other database will now allow empty Datasource URLs (e.g Spreadsheet, Text, etc.).

My solution for this would be a get database type and handle the test for PGSQL different then the rest. But I'm not sure which DATASOURCE_TYPE is used from /core/dbaccess/source/core/misc/dsntypes.hxx, because there is none for PostgreSQL.

2. It only allows an empty value when something is entered and then deleted, not at the time the page is set up. So enabling of the buttons is triggered through an event.

Currently I'm not familiar enough with the event system in LO to think of a good way to solve this.
Comment 12 Julien Nabet 2015-10-11 20:25:28 UTC
About dsn stuff, I submitted a patch to gerrit for dsn part:
https://gerrit.libreoffice.org/#/c/19310/
Comment 13 Commit Notification 2015-10-12 01:26:16 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=898aff0d190b09d16cce866909b64cf023cba43f

Related tdf#45775: PgSQL new DB allow empty Datasource

It will be available in 5.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 14 Commit Notification 2015-10-14 09:17:43 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-5-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=6b8479107482b180723d01d2fcd90120663435c5&h=libreoffice-5-0

Related tdf#45775: PgSQL new DB allow empty Datasource

It will be available in 5.0.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Robinson Tryon (qubit) 2015-12-14 06:30:58 UTC Comment hidden (obsolete)
Comment 16 Akshay Deep 2016-01-25 19:55:09 UTC
Can anyone help me how to compare ::dbaccess::DST_POSTGRES to m_eType variable in checkTestConnection() of "ConnectionPageSetup.cxx". m_eType gives error when used in the function's scope.
Comment 17 Lionel Elie Mamane 2016-01-25 20:05:06 UTC
(In reply to Akshay Deep from comment #16)
> Can anyone help me how to compare ::dbaccess::DST_POSTGRES to m_eType
> variable in checkTestConnection() of "ConnectionPageSetup.cxx". m_eType
> gives error when used in the function's scope.

I just tested it and I can use m_eType there, no error. The error you are seeing probably has some other cause. I'll gladly take a look if you attach the patch here.
Comment 18 Akshay Deep 2016-01-25 21:20:59 UTC
(In reply to Lionel Elie Mamane from comment #17)
> (In reply to Akshay Deep from comment #16)
> > Can anyone help me how to compare ::dbaccess::DST_POSTGRES to m_eType
> > variable in checkTestConnection() of "ConnectionPageSetup.cxx". m_eType
> > gives error when used in the function's scope.
> 
> I just tested it and I can use m_eType there, no error. The error you are
> seeing probably has some other cause. I'll gladly take a look if you attach
> the patch here.

@Lionel : Sorry, It was a typo error. I figured it out. 

Can you help me by helping me find code where the dialog is created and the buttons get enabled. I need to somehow perform a check that if the chosen db is PostgreSQl, then the next button should be enabled by default.

I am trying to look in "dbwizsetup.cxx".
Comment 19 Lionel Elie Mamane 2016-01-26 08:21:58 UTC
(In reply to Akshay Deep from comment #18)

> Can you help me by helping me find code where the dialog is created and the
> buttons get enabled. I need to somehow perform a check that if the chosen db
> is PostgreSQl, then the next button should be enabled by default.

> I am trying to look in "dbwizsetup.cxx".

Is comment 10 helpful?
Comment 20 Akshay Deep 2016-01-26 10:12:03 UTC
(In reply to Lionel Elie Mamane from comment #19)
> (In reply to Akshay Deep from comment #18)
> 
> > Can you help me by helping me find code where the dialog is created and the
> > buttons get enabled. I need to somehow perform a check that if the chosen db
> > is PostgreSQl, then the next button should be enabled by default.
> 
> > I am trying to look in "dbwizsetup.cxx".
> 
> Is comment 10 helpful?

Yes, It was really explanatory and I did what was required. Somehow, the NEXT button needs to be activated as soon as the dialog is created. And here I think I need to look into dbwizsetup.cxx from beginning of [comment 10]. I believe somewhere in ODbTypeWizDialogSetup::createPage()function of dbwizsetup.cxx, we can enable next button by default.
Comment 21 Commit Notification 2016-01-29 10:13:23 UTC
akki95 committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=90e18b514f639827091cee0f04892d999046eb18

tdf#45775 PgSQL new DB allow empty Datasource

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 22 Robinson Tryon (qubit) 2016-02-18 16:37:30 UTC Comment hidden (obsolete)