Adding the sql-stored-connector #58

KurtStam · 2017-09-08T05:16:53Z

Adding the connector which includes the VerifierExtension as well as the MetaDataExtension. The MetaDataExtension will need to go through another iteration to adhere to the latest conventions.

zregvart

I think some of the comments can be addressed in future PRs. Nothing that's really blocking this from getting merged.
One thing that we might want to do is to have a connector per database and use this one as a base for those. Not sure that citizen user will be interested in specifying driver class names. So having [oracle|postgresql|derby]-stored-procedure-connector that just extends from this might be better. That could open up bundling of Oracle driver as an option (as we would not be distributing it).

zregvart · 2017-09-08T07:17:47Z

connectors/salesforce-upsert-contact-connector/src/main/resources/camel-connector-schema.json

@@ -18,10 +18,10 @@
  },
  "componentProperties": {
    "loginUrl": { "kind": "property", "displayName": "Login Url", "group": "security", "label": "common,security", "required": true, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": false, "defaultValue": "https://login.salesforce.com", "description": "URL of the Salesforce instance by default set to https://login.salesforce.com" },
-    "clientId": { "kind": "property", "displayName": "Client Id", "group": "security", "label": "common,security", "required": true, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": false, "description": "OAuth Consumer Key of the connected app configured in the Salesforce instance setup. Typically a connected app needs to be configured but one can be provided by installing a package." },
+    "clientId": { "kind": "property", "displayName": "Client Id", "group": "security", "label": "common,security", "required": true, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": true, "description": "OAuth Consumer Key of the connected app configured in the Salesforce instance setup. Typically a connected app needs to be configured but one can be provided by installing a package." },


I think we wanted clientId to be non-secret see #55

hmm the only reason that would change is bc I ran the build. I'm working in the sql-stored-connector

Yeah, I saw this too with newer version of Camel, I guess the camel-maven-package/connector plugins might generate code differently between versions; and quite possibly that I've changed that from true to false by hand. I would keep it out of this PR if possible and regenerate in the next PR (I'll create one for upgrading Camel version).

zregvart · 2017-09-08T07:18:02Z

connectors/salesforce-upsert-contact-connector/src/main/resources/camel-connector-schema.json

    "clientSecret": { "kind": "property", "displayName": "Client Secret", "group": "security", "label": "common,security", "required": false, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": true, "description": "OAuth Consumer Secret of the connected app configured in the Salesforce instance setup." },
    "refreshToken": { "kind": "property", "displayName": "Refresh Token", "group": "security", "label": "common,security", "required": false, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": true, "description": "Refresh token already obtained in the refresh token OAuth flow. One needs to setup a web application and configure a callback URL to receive the refresh token or configure using the builtin callback at https://login.salesforce.com/services/oauth2/success or https://test.salesforce.com/services/oauth2/success and then retrive the refresh_token from the URL at the end of the flow. Note that in development organizations Salesforce allows hosting the callback web application at localhost." },
-    "userName": { "kind": "property", "displayName": "User Name", "group": "security", "label": "common,security", "required": false, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": false, "description": "Username used in OAuth flow to gain access to access token. It's easy to get started with password OAuth flow but in general one should avoid it as it is deemed less secure than other flows." },
+    "userName": { "kind": "property", "displayName": "User Name", "group": "security", "label": "common,security", "required": false, "type": "string", "javaType": "java.lang.String", "deprecated": false, "secret": true, "description": "Username used in OAuth flow to gain access to access token. It's easy to get started with password OAuth flow but in general one should avoid it as it is deemed less secure than other flows." },


Same for the userName

hmm the only reason that would change is bc I ran the build. I'm working in the sql-stored-connector

zregvart · 2017-09-08T07:22:53Z

connectors/sql-stored-connector/pom.xml

+    <!-- camel and spring boot compiler plugins -->
+    <dependency>
+      <groupId>org.apache.camel</groupId>
+      <artifactId>apt</artifactId>


Not sure we want apt to be in compile scope (perhaps provided or optional) or have them explicitly configured in the maven-compiler-plugin configuration see https://maven.apache.org/plugins/maven-compiler-plugin/compile-mojo.html#annotationProcessorPaths

@zregvart We seem to be doing this on all connectors, so we should probably address that across all connectors. Shall I open a Jira just for this?

Yeah, I think it would benefit to have this in the parent POM also

zregvart · 2017-09-08T07:23:11Z

connectors/sql-stored-connector/pom.xml

+    </dependency>
+    <dependency>
+      <groupId>org.springframework.boot</groupId>
+      <artifactId>spring-boot-configuration-processor</artifactId>


Same about annotation processing as above

zregvart · 2017-09-08T07:24:50Z

connectors/sql-stored-connector/pom.xml

+        <artifactId>maven-resources-plugin</artifactId>
+        <version>3.0.1</version>
+        <configuration>
+          <encoding>UTF-8</encoding>


I wonder if we should also use https://github.com/basepom/basepom for connectors, or perhaps have a connectors-parent POM (no need to change in this PR), so some of these encodings (and others that we forget) are set beforehand

zregvart · 2017-09-08T07:30:13Z

...tored-connector/src/main/java/io/syndesis/connector/SqlStoredConnectorMetaDataExtension.java

+
+        Connection connection = null;
+        ResultSet columnSet = null;
+        try {


ditto for try-with-resources

zregvart · 2017-09-08T07:34:49Z

...tored-connector/src/main/java/io/syndesis/connector/SqlStoredConnectorVerifierExtension.java

+                Class.forName(parameters.get("driver").toString());
+            } catch (Exception e) {
+                Enumeration<Driver> drivers = DriverManager.getDrivers();
+                StringBuffer supportedDrivers = new StringBuffer("[");


Perhaps use StringJoiner? Something like:

Collections.list(DriverManager.getDrivers()) .stream() .map(Class::getName) .collect(Collectors.joining(", ", "[", "]"));

zregvart · 2017-09-08T07:40:34Z

...tored-connector/src/main/java/io/syndesis/connector/SqlStoredConnectorVerifierExtension.java

+                    parameters.get("user").toString(), 
+                    parameters.get("password").toString());
+
+            if (connection == null) {


Not sure this can happen according to the docs it should not return null.

zregvart · 2017-09-08T07:45:35Z

...tored-connector/src/main/java/io/syndesis/connector/SqlStoredConnectorVerifierExtension.java

+
+    private void verifyCredentials(ResultBuilder builder, Map<String, Object> parameters) {
+        try {
+            Class.forName(parameters.get("driver").toString());


I wonder if we should support non JDBC 4.0 drivers. With JDBC 4.0 this is no longer necessary. DriverManager will try to locate an appropriate driver via ServiceLoader, see the docs.

zregvart · 2017-09-08T07:53:26Z

examples/sql-stored-example/pom.xml

+        <artifactId>exec-maven-plugin</artifactId>
+        <version>1.5.0</version>
+        <configuration>
+          <mainClass>io.syndesis.example.TwitterPingCheckMain</mainClass>


typo, should be SqlStoredPingCheckMain?

KurtStam · 2017-09-12T08:34:52Z

@zregvart I think your comments on the pom above should be addressed in a separate issue as this applies to all the connectors. #62

zregvart

👍

pure-bot · 2017-09-12T08:39:14Z

Pull request approved by @zregvart - applying approved label

KurtStam force-pushed the sql-stored-connector branch from 976da29 to dd7037d Compare September 8, 2017 05:22

zregvart reviewed Sep 8, 2017

View reviewed changes

KurtStam force-pushed the sql-stored-connector branch from dd7037d to 1950f0a Compare September 12, 2017 08:18

Adding sql-stored-connector

5344293

KurtStam force-pushed the sql-stored-connector branch from 1950f0a to 5344293 Compare September 12, 2017 08:33

KurtStam mentioned this pull request Sep 12, 2017

Pull some build sections in parent pom if possible #62

Open

zregvart approved these changes Sep 12, 2017

View reviewed changes

pure-bot bot added the approved label Sep 12, 2017

pure-bot bot merged commit 6abe14c into syndesisio:master Sep 12, 2017

syndesis-bot mentioned this pull request Nov 15, 2017

Pull some build sections in parent pom if possible syndesisio/syndesis#302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding the sql-stored-connector #58

Adding the sql-stored-connector #58

KurtStam commented Sep 8, 2017

zregvart left a comment

zregvart Sep 8, 2017

KurtStam Sep 12, 2017

zregvart Sep 12, 2017

zregvart Sep 8, 2017

KurtStam Sep 12, 2017

zregvart Sep 8, 2017

KurtStam Sep 11, 2017

zregvart Sep 11, 2017

zregvart Sep 8, 2017

zregvart Sep 8, 2017

zregvart Sep 8, 2017

zregvart Sep 8, 2017

zregvart Sep 8, 2017

zregvart Sep 8, 2017

zregvart Sep 8, 2017 •

edited

KurtStam commented Sep 12, 2017 •

edited

zregvart left a comment

pure-bot bot commented Sep 12, 2017

Adding the sql-stored-connector #58

Adding the sql-stored-connector #58

Conversation

KurtStam commented Sep 8, 2017

zregvart 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

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

zregvart Sep 8, 2017 • edited

Choose a reason for hiding this comment

KurtStam commented Sep 12, 2017 • edited

zregvart left a comment

Choose a reason for hiding this comment

pure-bot bot commented Sep 12, 2017

zregvart Sep 8, 2017 •

edited

KurtStam commented Sep 12, 2017 •

edited