SLVS-1455 Make Id of type URL #5698

vnaskos-sonar · 2024-09-20T14:35:02Z

What

Store the https://sonarcloud.io/organizations/OrganizationKey instead of the sonarcloud organization as connection ID.

Why

This is pre-work to remove the prefixes (sc| and sq|) from ConnectionIdHelper. Connections should be identifiable by their Id.

How

ConnectionInfo is a UI model that is used to display information on the UI and act as a DTO.
1.1. The ConnectionInfo.Id is either the sonarqube uri or the sonarcloud organization.
ServerConnection is a backend model and it should not be used directly on the UI.
1.1. The ServerConnection.Id is always a uri
...Adapter should be able to convert UI models to backend models (eg. ConnectionInfo to ServerConnection)
...Repository should only know/reference backend models and never UI models.

Expected results

connections.json: All connections Id should be URLs
- sonarqube: http://localhost:9000
- sonarcloud: https://sonarcloud.io/organizations/OrganizationKey
binding.config: ServerConnectionId should be the same as in connection.json
On the UI (Manage Binding, Manage connections, New connection) the sonarcloud entries should display only the organizationKey

gabriela-trutan-sonarsource

Please see the feedback contained in the comments.

Additionally, the id for SonarQube receives a trailing / (even if in the UI I did not add it), but the SonarCloud doesn't have it. Should we make them consistent?

src/ConnectedMode/SlCoreConnectionAdapter.cs

src/ConnectedMode/ConnectionInfo.cs

src/Core/Binding/ServerConnection.cs

src/Core/CoreStrings.resx

gabriela-trutan-sonarsource · 2024-09-23T14:41:10Z

src/ConnectedMode/ConnectionInfo.cs

@@ -33,11 +33,12 @@ public record ConnectionInfo(string Id, ConnectionServerType ServerType)
 {


I think we should add documentation for what the Id is expected to be and that it is different than the ServerConnection

src/ConnectedMode/SlCoreConnectionAdapter.cs

src/Core.UnitTests/Binding/ServerConnectionTests.cs

gabriela-trutan-sonarsource · 2024-09-25T07:33:26Z

src/ConnectedMode/SlCoreConnectionAdapter.cs

@@ -91,7 +91,11 @@ public Task<AdapterResponseWithData<List<OrganizationDisplay>>> GetOrganizations

            try
            {
-                var credentials = MapCredentials(credentialsModel?.ToICredentials());
+                if (credentialsModel == null)


We don't need this explicit check. If it is null, there will be a null reference exception that will be caught by the catch block.

In case the credentialsModel is null the ToICredentials will throw a generic null exception. In this case I believe it's better to be proactive and add back the .? so the exception we get becomes specific to the lack of credentials.

gabriela-trutan-sonarsource · 2024-09-25T07:35:32Z

src/ConnectedMode/ConnectionInfo.cs

@@ -37,7 +37,7 @@ public static ConnectionInfo From(ServerConnection serverConnection)
        {
            ServerConnection.SonarQube sonarQubeConnection => new ConnectionInfo(sonarQubeConnection.Id, ConnectionServerType.SonarQube),
            ServerConnection.SonarCloud sonarCloudConnection => new ConnectionInfo(sonarCloudConnection.OrganizationKey, ConnectionServerType.SonarCloud),
-            _ => null
+            _ => throw new ArgumentException(Resources.UnexpectedConnectionType)


Please add unit test

It is not technically possible to add a unit test for this case. Technically, this branch is unreachable as the ServerConnection constructor is private, so a ServerConnection can be either SonarQube or SonarCloud. It is also not possible to remove the default case without having a warning, as the C# compiler can't properly recognize exhaustive switch.

gabriela-trutan-sonarsource · 2024-09-25T07:35:37Z

src/ConnectedMode/SlCoreConnectionAdapter.cs

@@ -216,7 +216,7 @@ private static Either<TransientSonarQubeConnectionDto, TransientSonarCloudConnec
                new TransientSonarQubeConnectionDto(connectionInfo.Id, credentialsDto)),
            ConnectionServerType.SonarCloud => Either<TransientSonarQubeConnectionDto, TransientSonarCloudConnectionDto>.CreateRight(
                new TransientSonarCloudConnectionDto(connectionInfo.Id, credentialsDto)),
-            _ => null
+            _ => throw new ArgumentException(Resources.UnexpectedConnectionType)


Please add unit test

gabriela-trutan-sonarsource · 2024-09-25T07:35:40Z

src/ConnectedMode/SlCoreConnectionAdapter.cs

@@ -230,7 +230,7 @@ private static Either<TransientSonarQubeConnectionDto, TransientSonarCloudConnec
                new TransientSonarQubeConnectionDto(sonarQubeConnection.Id, credentials)),
            ServerConnection.SonarCloud sonarCloudConnection => Either<TransientSonarQubeConnectionDto, TransientSonarCloudConnectionDto>.CreateRight(
                new TransientSonarCloudConnectionDto(sonarCloudConnection.OrganizationKey, credentials)),
-            _ => null
+            _ => throw new ArgumentException(Resources.UnexpectedConnectionType)


Please add unit test

gabriela-trutan-sonarsource · 2024-09-25T07:56:47Z

src/ConnectedMode/SlCoreConnectionAdapter.cs

+
+        return connectionInfo.ServerType switch
+        {
+            ConnectionServerType.SonarQube => Either<TransientSonarQubeConnectionDto, TransientSonarCloudConnectionDto>.CreateLeft(


Should we consider adding a comment here or in the caller method to point out that the organizationKey is expected to be null, when validating a transient connection (a connection that is not yet finished)?

I agree that it would make it a bit more clear. However, I am not sure whether this is the correct place. This requirement comes from the SLCore API, which is already documented there, and on our end, we have unit tests to verify that this behavior is respected.

sonarqubecloud · 2024-09-25T10:33:26Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
93.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

vnaskos-sonar force-pushed the vn/id-to-uri branch 2 times, most recently from 77da67c to 2d08fca Compare September 23, 2024 11:14

vnaskos-sonar marked this pull request as ready for review September 23, 2024 13:18

vnaskos-sonar requested a review from gabriela-trutan-sonarsource September 23, 2024 13:18

gabriela-trutan-sonarsource requested changes Sep 23, 2024

View reviewed changes

vnaskos-sonar added 10 commits September 24, 2024 16:18

Change sonarcloud ServerConnection Id to Uri

81a1e61

Prevent leaking UI models to backend

02dc1f9

Continue on separation of concerns

553831e

Replace hardcoded url with Resource

db71335

Fix rebase issues

e0b7b61

Validation of new SonarCloud connection does not have an organization

db52a0c

Throw exception if credentialsModel is null

f4b868e

Throw exception in case of unknown connection type

955edfe

Reuse CoreStrings.SonarCloudUrl

290d104

Add documentation

734aa31

vnaskos-sonar force-pushed the vn/id-to-uri branch from 2d08fca to 734aa31 Compare September 25, 2024 06:33

Fix test

56b85a7

vnaskos-sonar requested a review from gabriela-trutan-sonarsource September 25, 2024 07:00

gabriela-trutan-sonarsource approved these changes Sep 25, 2024

View reviewed changes

Replace redundant check with null conditional operator

6f9ed50

vnaskos-sonar merged commit 9c754e2 into feature/new-connected-mode Sep 25, 2024
2 checks passed

vnaskos-sonar deleted the vn/id-to-uri branch September 25, 2024 10:33

gabriela-trutan-sonarsource pushed a commit that referenced this pull request Sep 27, 2024

SLVS-1455 Make Id of type URL (#5698)

d69418f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SLVS-1455 Make Id of type URL #5698

SLVS-1455 Make Id of type URL #5698

vnaskos-sonar commented Sep 20, 2024 •

edited by jira bot

Loading

gabriela-trutan-sonarsource left a comment

gabriela-trutan-sonarsource Sep 23, 2024

gabriela-trutan-sonarsource Sep 25, 2024

vnaskos-sonar Sep 25, 2024

gabriela-trutan-sonarsource Sep 25, 2024

vnaskos-sonar Sep 25, 2024

gabriela-trutan-sonarsource Sep 25, 2024

gabriela-trutan-sonarsource Sep 25, 2024

gabriela-trutan-sonarsource Sep 25, 2024

vnaskos-sonar Sep 25, 2024

sonarqubecloud bot commented Sep 25, 2024

		@@ -33,11 +33,12 @@ public record ConnectionInfo(string Id, ConnectionServerType ServerType)
		{

SLVS-1455 Make Id of type URL #5698

SLVS-1455 Make Id of type URL #5698

Conversation

vnaskos-sonar commented Sep 20, 2024 • edited by jira bot Loading

What

Why

How

Expected results

gabriela-trutan-sonarsource left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 25, 2024

Quality Gate passed

vnaskos-sonar commented Sep 20, 2024 •

edited by jira bot

Loading