SLVS-1431 Add server connection json model and mapper #5658
Conversation
…aps the model from the logic model to the json model and vice-versa
terraform-techuser
changed the title
|
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
| @@ -25,7 +25,7 @@ public abstract class ServerConnection | |||
| internal static readonly ServerConnectionSettings DefaultSettings = new(true); | |||
|
|
|||
| public string Id { get; } | |||
| public ServerConnectionSettings Settings { get; } | |||
| public ServerConnectionSettings Settings { get; set; } | |||
There was a problem hiding this comment.
Why was the setter added?
There was a problem hiding this comment.
Because in the repository we have TryUpdateSettingsById, which requires to add the settings to an existing ServerConnection model
There was a problem hiding this comment.
I don't know if we should do that. Feels like a thread safety issue. The model can be modified from another thread. Could we make it immutable? Would that have big implications on other parts of the code?
There was a problem hiding this comment.
In the repository, I am reading the file and then retrieving the ServerConnection model for the provided Id. I then have to update the settings:
I either allow modification of the settings or I need to create a new instance of the ServerConnection (which is immutable), then remove it from the list, then re-add the new instance. This feels a lot more error-prone and unexpected.
I could make instead the IsSmartNotificationsEnabled editable (add a setter for it). Also I don't think we should be too paranoid about the multi-threading. Because all the write operations to the connections should only happen via the UI. And the UI will be blocked while anything in the connection is modified
gabriela-trutan-sonarsource
merged commit 61c9605
into
feature/new-connected-mode
gabriela-trutan-sonarsource
deleted the
gt/json-server-connection-model
branch
SLVS-1431