GDB-5393 Refactor repository and server clients configuration #127

teodossidossev · 2021-01-25T13:01:33Z

Provides configuration builder and unifies configuration creation.

svilenvelikov · 2021-01-26T09:29:08Z

src/http/client-config-builder.js

+   * to all server requests
+   * @return {ServerClientConfig}
+   */
+  static serverConfig(endpoint) {


Remove the static in order to be consistent with other methods. And same for the repositoryConfig method

Or maybe make the other two private

Both creator methods are static to be consistent with the other builders in the driver.

svilenvelikov · 2021-01-26T09:38:12Z

src/http/client-config.js

-   * server is secured and username and passwords are provided.
-   * @param {boolean} [useBasicAuth] if use Basic Auth when authenticating
+   * Client configuration constructor.
+   * Initializes [headers]{@link ClientConfig#headers} and


Well, this constructor doesn't do what you've described

svilenvelikov · 2021-01-26T09:42:05Z

src/http/client-config.js

@@ -62,6 +56,7 @@ class ClientConfig {
   */
  setUsername(username) {
    this.username = username;
+    this.useBasicAuthentication();


Why? It's is quite unusual that you set this here in the username setter, and also in the password setter. Not to mention that if I do config.setUsername('me'), it'll populate the Authorization header with wrong data.

Also setBasicAuthentication does the same and I think it's not correct to have a setter which does something else underneath. IMO this basic auth configration should be made explicit and should not make the user wonder how to configure the client. Think if it wouldn't be better to remove the user/pass setters in favor of useBasicAuthentication which should also require the user/pass as arguments and does all this explicit.

svilenvelikov · 2021-01-26T11:11:43Z

test-e2e/tests/config.js

-const {RepositoryClientConfig} = require('graphdb').repository;
-const {RDFMimeType} = require('graphdb').http;
-const {ServerClientConfig} = require('graphdb').server;
+/* eslint-disable max-len */


Can't you just format the line instead?!

svilenvelikov · 2021-01-26T11:30:27Z

test-e2e/tests/auth-server-client.spec.js

@@ -1,13 +1,16 @@
+/* eslint "require-jsdoc": off*/


Better just exclude the rule for the tests. See https://eslint.org/docs/user-guide/configuring#disabling-rules-only-for-a-group-of-files

svilenvelikov · 2021-01-26T11:39:40Z

test/repository/rdf-repository-client-adding-data.spec.js

@@ -1,5 +1,7 @@
+/* eslint-disable max-len */
+/* eslint "require-jsdoc": off*/


remove these: format the long lines and disable the jsdoc rule for tests

svilenvelikov · 2021-02-09T07:34:06Z

src/http/client-config-builder.js

+   * to all server requests
+   * @return {RepositoryClientConfig}
+   */
+  buildRepositoryClientConfig(endpoint) {


private maybe? And the same for the buildServerClientConfig

svilenvelikov · 2021-02-09T07:38:41Z

src/http/client-config-builder.js

+ * @class
+ * @author Teodossi Dossev
+ */
+class ClientConfigBuilder {


I still can't get the purpose of this so called builder, which seems more like a factory to me. But lets assume we decide to leave it. In such a case shouldn't we need to hide the RepositoryClientConfig and ServerClientConfig constructors and force the client to get their instances only through the factory?! Otherwise would be more confusing and unclear to me as a client how should I get a client instance and what's the problem with the direct instantiation.

And another question. As long as the server and repository configs are situated in their respective packages, why this is placed inside the http package?!

svilenvelikov · 2021-02-09T07:54:30Z

src/http/client-config.js

   */
-  setBasicAuthentication(basicAuth) {
+  enableBasicAuthentication(basicAuth) {


This method is quite confusing. It sais enableBasicAuthentication and yet it seems that it might not do it. Can you please clarify it?

svilenvelikov · 2021-02-09T07:56:00Z

src/http/client-config.js

-    if (this.basicAuth) {
-      const credentials = `${this.username}:${this.pass}`;
-      this.headers['Authorization'] = `Basic ${btoa(credentials)}`;
+  enableGdbTokenAuthentication(gdbTokenAuth) {


Same note as for enableBasicAuthentication

svilenvelikov · 2021-02-09T08:09:19Z

src/server/server-client.js

      .then((user) => {
+        this.setLoggedUser(user);


You execute a login with logged user and after that you set logged user... it's strange. Is the user logged in before login execution and what happens with it after the login?

svilenvelikov · 2021-02-09T08:10:38Z

src/server/server-client.js

+   * Logged user getter.
+   * @return {User} user
+   */
+  getLoggedUser() {


Why not just getUser and setUser? Otherwise it's confusing.

svilenvelikov · 2021-02-09T08:15:35Z

src/service/authentication-service.js

-   * @return {AuthenticationService}
+   * Authentication type getter
+   * @param {ClientConfig} clientConfig concrete client configuration
+   * @return {BasicAuthentication|GdbTokenAuthentication} conctrete


typo in conctrete

svilenvelikov · 2021-02-09T08:19:49Z

src/service/transaction-service.js

@@ -79,7 +79,8 @@ class TransactionService extends Service {
   */
  getTransactionalClientConfig(locationUrl) {
    const config = this.repositoryClientConfig;
-    return ClientConfigBuilder.repositoryConfig(config.getEndpoint())
+    return new ClientConfigBuilder()
+      .repositoryConfig(config.getEndpoint())


Here's an example of why the ClientConfigBuilder is confusing. You get an instance of the RepositoryClientConfig builder by just providing the only required parameter and then continue with its initialization. It's seems just redundant to me.

svilenvelikov · 2021-02-09T08:42:46Z

test/repository/rdf-repository-client-query.spec.js

@@ -217,7 +215,8 @@ describe('RDFRepositoryClient - query', () => {
      .setOffset(0)
      .setTimeout(5);

-    const expectedData = 'query=select%20*%20where%20%7B%3Fs%20%3Fp%20%3Fo%7D&queryLn=sparql&infer=true&distinct=true&limit=100&offset=0&timeout=5';
+    const expectedData = 'query=select%20*%20where%20%7B%3Fs%20%3Fp%20%3Fo%7D' +


pff this formatting in the tests was quite unneeded IMO

Provides configuration builder and unifies configuration creation.

Provides authentication factory. Authentication service refactor to decouple user state Minor sonar bug fixes Test refactored according code changes New eslint rules for test files

update README.md

sonarcloud · 2021-02-10T07:53:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

teodossidossev force-pushed the GDB-5393-refactor-server-client-configs branch 2 times, most recently from 38c4795 to 50f574f Compare January 25, 2021 13:57

teodossidossev requested a review from svilenvelikov January 25, 2021 14:13

svilenvelikov reviewed Jan 26, 2021

View reviewed changes

svilenvelikov requested changes Jan 26, 2021

View reviewed changes

svilenvelikov requested changes Feb 9, 2021

View reviewed changes

svilenvelikov previously approved these changes Feb 9, 2021

View reviewed changes

teodossidossev dismissed svilenvelikov’s stale review via 6ee28ab February 10, 2021 07:09

teodossidossev added 5 commits February 10, 2021 09:09

GDB-5393 Refactor repository and server clients configuration

adf42af

Provides configuration builder and unifies configuration creation.

GDB-5393 Refactor repository and server clients configuration

8bb0de3

Provides authentication factory. Authentication service refactor to decouple user state Minor sonar bug fixes Test refactored according code changes New eslint rules for test files

GDB-5393 Refactor repository and server clients configuration

d7dd3fe

GDB-5393 Refactor repository and server clients configuration

8459dc9

GDB-5393 Refactor repository and server clients configuration

1da2397

update README.md

teodossidossev force-pushed the GDB-5393-refactor-server-client-configs branch from 6ee28ab to 9cdd7e0 Compare February 10, 2021 07:11

svilenvelikov previously approved these changes Feb 10, 2021

View reviewed changes

GDB-5393 Refactor repository and server clients configuration

3241953

teodossidossev dismissed svilenvelikov’s stale review via 3241953 February 10, 2021 07:52

teodossidossev force-pushed the GDB-5393-refactor-server-client-configs branch from 9cdd7e0 to 3241953 Compare February 10, 2021 07:52

svilenvelikov approved these changes Feb 10, 2021

View reviewed changes

teodossidossev merged commit ae13c41 into master Feb 10, 2021

teodossidossev deleted the GDB-5393-refactor-server-client-configs branch February 10, 2021 08:03

teodossidossev mentioned this pull request Feb 12, 2021

Refactor server and client configurations #124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GDB-5393 Refactor repository and server clients configuration #127

GDB-5393 Refactor repository and server clients configuration #127

teodossidossev commented Jan 25, 2021

svilenvelikov Jan 26, 2021 •

edited

Loading

svilenvelikov Jan 26, 2021

teodossidossev Jan 26, 2021

svilenvelikov Jan 26, 2021

svilenvelikov Jan 26, 2021

svilenvelikov Jan 26, 2021

svilenvelikov Jan 26, 2021

svilenvelikov Jan 26, 2021

svilenvelikov Jan 26, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021 •

edited

Loading

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

svilenvelikov Feb 9, 2021

sonarcloud bot commented Feb 10, 2021

		@@ -1,5 +1,7 @@
		/* eslint-disable max-len */
		/* eslint "require-jsdoc": off*/

GDB-5393 Refactor repository and server clients configuration #127

GDB-5393 Refactor repository and server clients configuration #127

Conversation

teodossidossev commented Jan 25, 2021

svilenvelikov Jan 26, 2021 • edited Loading

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

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

svilenvelikov Feb 9, 2021 • edited Loading

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

sonarcloud bot commented Feb 10, 2021

svilenvelikov Jan 26, 2021 •

edited

Loading

svilenvelikov Feb 9, 2021 •

edited

Loading