Bug 157260 - Test connection is not using the port specified in connection string for PostgreSQL using 5432 instead
Summary: Test connection is not using the port specified in connection string for Post...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
7.6.0.3 release
Hardware: All All
: high major
Assignee: Not Assigned
URL:
Whiteboard: target:24.2.0 target:7.6.3
Keywords: bibisected, regression
Depends on:
Blocks:
 
Reported: 2023-09-15 13:49 UTC by Rob
Modified: 2023-09-26 17:43 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
Snapshot of the error (13.54 KB, image/png)
2023-09-15 13:50 UTC, Rob
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rob 2023-09-15 13:49:25 UTC
Description:
Editing the connection information of an existing odb file then testing the connection, the code seems to use the default port 5432 instead of the port specified in the connection string. When you first create the file is seems okay but editing later seems to cause the bug when you attempt to use test the connection.

Steps to Reproduce:
1. Operate a PostgreSQL database on the non-default port e.g. 10050
2. Create a connection  
3. Save the file
4. Right click to get the content menu
5. Slect Database -> Propeties -> Test connection

Actual Results:
The connection fails using port 5432 but the connection string does not use that port

Expected Results:
It should use the port in the connection string to test the connection


Reproducible: Always


User Profile Reset: No

Additional Info:
I have an image and will try to attached it
Comment 1 Rob 2023-09-15 13:50:55 UTC
Created attachment 189611 [details]
Snapshot of the error
Comment 2 Rob 2023-09-15 15:13:21 UTC
This issue does not happen in version:

Version: 7.5.5.2 (X86_64) / LibreOffice Community
Build ID: ca8fe7424262805f223b9a2334bc7181abbcbf5e
CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win
Locale: en-CA (en_CA); UI: en-US
Calc: CL threaded
Comment 3 Rob 2023-09-15 15:51:35 UTC
The issue still exists in the latest version and seems to be isolated to the code that handles 'Test Connection'

Version: 7.6.1.2 (X86_64) / LibreOffice Community
Build ID: f5defcebd022c5bc36bbb79be232cb6926d8f674
CPU threads: 8; OS: Windows 10.0 Build 19045; UI render: Skia/Vulkan; VCL: win
Locale: en-CA (en_CA); UI: en-US
Calc: CL threaded
Comment 4 Alex Thurgood 2023-09-20 10:38:41 UTC
Confirming with:

Version: 7.6.1.2 (AARCH64) / LibreOffice Community
Build ID: f5defcebd022c5bc36bbb79be232cb6926d8f674
CPU threads: 8; OS: Mac OS X 13.4; UI render: Skia/Raster; VCL: osx
Locale: fr-FR (fr_FR.UTF-8); UI: fr-FR
Calc: threaded

I get the following error :

Couldn't establish database connection to 'sdbc:postgresql: host='localhost' port='5435' dbname='alex' port='5432''
could not connect to server: Connection refused
	Is the server running on host "localhost" (::1) and accepting
	TCP/IP connections on port 5432?
could not connect to server: Connection refused
	Is the server running on host "localhost" (fe80::1) and accepting
	TCP/IP connections on port 5432?
could not connect to server: Connection refused
	Is the server running on host "localhost" (127.0.0.1) and accepting
	TCP/IP connections on port 5432?

==> regression
Comment 5 Alex Thurgood 2023-09-20 10:41:08 UTC
Probably an unconsidered consequence of the user connection string code change in the connection dialog.
Comment 6 Alex Thurgood 2023-09-20 10:46:28 UTC
(In reply to Alex Thurgood from comment #5)
> Probably an unconsidered consequence of the user connection string code
> change in the connection dialog.

More particularly, the following:

commit 278ab9f4985db540ff4b8872c2d03a9bd75324dc
Comment 7 Alex Thurgood 2023-09-20 10:47:49 UTC
@Nirnay : any ideas ?
Comment 8 Alex Thurgood 2023-09-20 11:06:11 UTC
If, after testing the connection, getting the error message, clicking OK out of the message, and then mistakenly saving the ODB file, the connection string gets saved as:

 host='localhost' port='5435' dbname='alex' port='5432'

which inevitably then fails when trying to load any tables, queries, etc when the ODB file is re-opened.

In other words, there is file corruption, at least insofar as the connection string is then stored incorrectly within the file.

This is the law of unintended consequences in play, at its finest !
Comment 9 Julien Nabet 2023-09-20 11:31:17 UTC
(In reply to Alex Thurgood from comment #6)
> (In reply to Alex Thurgood from comment #5)
> > Probably an unconsidered consequence of the user connection string code
> > change in the connection dialog.
> 
> More particularly, the following:
> 
> commit 278ab9f4985db540ff4b8872c2d03a9bd75324dc

The link gives 404
Comment 10 Alex Thurgood 2023-09-20 16:08:43 UTC
(In reply to Julien Nabet from comment #9)


> > More particularly, the following:
> > 
> > commit 278ab9f4985db540ff4b8872c2d03a9bd75324dc
> 
> The link gives 404


Sorry about that, try:

https://git.libreoffice.org/core/+/afe99617707c92460e66486c0057ef327e8aa017%5E%21
Comment 11 Julien Nabet 2023-09-23 14:45:38 UTC
Ok, I gave a try on pc Debian x86-64 with master sources updated today and could reproduce this.
Comment 12 Julien Nabet 2023-09-23 15:28:25 UTC
I confirm it's indeed related to https://cgit.freedesktop.org/libreoffice/core/commit/?id=afe99617707c92460e66486c0057ef327e8aa017

I've submitted this patch for review:
https://gerrit.libreoffice.org/c/core/+/157193

In brief, I just test if connection url already contains "port=".
First I thought about just "port" but then thought that some database or host may contain "port", so used "port=".
But perhaps it may also contain "port =" (so with a space) and the patch won't work :-(.
I'll try to figure out something else but in the meanwhile, the patch can still be a band-aid.
Comment 13 Julien Nabet 2023-09-23 21:12:51 UTC
(In reply to Julien Nabet from comment #12)
> ...
> But perhaps it may also contain "port =" (so with a space) and the patch
> won't work :-(.
> I'll try to figure out something else but in the meanwhile, the patch can
> still be a band-aid.
Re thinking about this, ideally we'd need some parser since as I put in comment:
- keys are not ordered
- sometimes, only a space separate the pairs of key/values, sometimes it's a semicolon
- perhaps you can put "key=value" but also "key = value" (I may be wrong but still there are the 2 previous quoted issues) 

So let's keep this naive band-aid fix.
Of course if someone wants to implement a better solution, don't hesitate ! :-)
Comment 14 Commit Notification 2023-09-23 22:29:17 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/3399854b66bb1fb175b8504c964008e53167ca7b

tdf#157260: don't add a "port" param if already present in the connection DB url

It will be available in 24.2.0.

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

Affected users are encouraged to test the fix and report feedback.
Comment 15 Julien Nabet 2023-09-24 06:59:46 UTC
Cherry-pick for 7.6 waiting for review here:
https://gerrit.libreoffice.org/c/core/+/157129
Comment 16 Commit Notification 2023-09-25 07:20:00 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "libreoffice-7-6":

https://git.libreoffice.org/core/commit/93fa91130cf41ae3cb2f8ef6eb596d34db55c7d1

tdf#157260: don't add a "port" param if already present in the connection DB url

It will be available in 7.6.3.

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

Affected users are encouraged to test the fix and report feedback.
Comment 17 Rob 2023-09-25 19:14:27 UTC
In python a regex as shown below "seemed to help"

import re
var1 = re.sub('\\s*=\\s*','=','host = 10050')

I am not sure of the equivalent in c/c++
Comment 18 Julien Nabet 2023-09-25 20:10:57 UTC
(In reply to Rob from comment #17)
> In python a regex as shown below "seemed to help"
> 
> import re
> var1 = re.sub('\\s*=\\s*','=','host = 10050')
> 
> I am not sure of the equivalent in c/c++

"rURL" is an OUString (a specific type defined in LO) and I don't think there's regexp mechanism for them (but perhaps I'm wrong).
For just one space, we could also avoid regexp mechanism and use a dumb double condition
if (pPortNumber && pPortNumber->GetValue() && (rURL.indexOf("port=") == -1) && (rURL.indexOf("port =") == -1))

since there's few chance people put more than 1 space and this part of code doesn't need perf optim.


Also perhaps the right way would be to override "extractHostNamePort" so each DB implementation (in subdirs connectivity/source/drivers/) would call the ad-hoc function of DB API (Eg: in Postgresql, there's "PQconninfoParse" method)

I mean why trying to re-create a parser since DBs have already a function to do it?
Comment 19 Rob 2023-09-26 17:35:06 UTC
If PQconninfoParse would help then that might be a good option.
Comment 20 Rob 2023-09-26 17:43:07 UTC
I just tested the night build for 2023-09-26.

Results are positive i.e. bug squashed.