added error messages factory #15832
added error messages factory #15832
Conversation
…11322_error_message
…nto akorotkov/11322_error_message # Conflicts: # airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/errors/ErrorMessageFactory.java # airbyte-integrations/bases/base-java/src/main/java/io/airbyte/integrations/base/errors/utils/ConnectorType.java
…11322_error_message
…nto akorotkov/11322_error_message
…nto akorotkov/11322_error_message
…nto akorotkov/11322_error_message
…nto akorotkov/11322_error_message
…11322_error_message
…11322_error_message
…11322_error_message
|
@grishick the tests for some connectors fail not because of my changes, but because for most of them the changes were mergrd without running the tests. For example, for redshift-destination, no changes were added to secrets in Google Secret Manager. For mysql-destination-strict-encrypt - no changes were added to the transformation of types (when added to mysql-destination they were not added here) and several more connectors with small errors. |
There was a problem hiding this comment.
There's a few things that still need to be reviewed, the things that seem important are mainly on:
- removing the use of
varespecially in areas where the code explicitly defines a type - provide a link to each of the connectors state code since it wouldn't be super useful to report a state code for a connector only for ourselves to have difficult finding these codes
- remove the
MySqlTestDataComparatorwork since that does not seem relevant to the error messages work
If there's a viewpoint that wasn't clear, let's work together to get this over the finish line. There's a lot a great work here and would love for this to benefit our users
airbyte-db/db-lib/src/main/java/io/airbyte/db/mongodb/MongoDatabase.java
Outdated
Show resolved
Hide resolved
airbyte-db/db-lib/src/main/java/io/airbyte/db/jdbc/DefaultJdbcDatabase.java
Show resolved
Hide resolved
...rs/destination-gcs/src/main/java/io/airbyte/integrations/destination/gcs/GcsDestination.java
Outdated
Show resolved
Hide resolved
| if (!databaseNames.contains(databaseName) && !databaseName.equals(database.getName())) { | ||
| throw new MongodbDatabaseException(databaseName); | ||
| } | ||
| return new AirbyteConnectionStatus().withStatus(AirbyteConnectionStatus.Status.SUCCEEDED); | ||
| } catch (final ConnectionErrorException e) { | ||
| var messages = getErrorMessage(e.getStateCode(), e.getErrorCode(), e.getExceptionMessage(), e); |
There was a problem hiding this comment.
Unless I'm mistaken, isn't it possible for ConnectionErrorException to be created with the constructor ConnectionErrorException(errorMessage) and so e.getErrorCode() will not exist?
There was a problem hiding this comment.
Yes, that's right, it's possible. But in processing, in the ErrorMessage.getErrorMessage method, I'll process it, and if the error code is empty, it won't change the readability of the processed error message.
There was a problem hiding this comment.
Wouldn't that mean that you can be passing in an uninitialized int into getErrorMessage? That would cause an error cause in MongoDbSource you create a new ConnectionErrorException(String exceptionMessage) but that does not include an errorCode
When I tested this quick snipped out in my IDE this was giving my an error compiling which mimics how getErrorMessage works, correct me if I'm wrong
@Test
void testUninitialzedIntPassed() {
final int number;
System.out.println(runTest("hi", number, "world"));
}
public String runTest(final String first, final int num, final String second) {
final String temp = num == 0 ? "" : " TEST";
return first + temp + second;
};| ((ObjectNode) config).put(JdbcUtils.HOST_KEY, "localhost2"); | ||
| final AirbyteConnectionStatus actual = source.check(config); | ||
| assertEquals(Status.FAILED, actual.getStatus()); | ||
| assertTrue(actual.getMessage().contains("State code: 28000; Error code: 200028;")); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
It seems the messaging was lost, the comment was not intended to say that Charles has a PR that would potentially conflict with the changes here. The message was that here are the list of tests that should be covered in each of the different connectors. So namely invalid:
- host
- port
- username
- password
- database
- schema
The reason for asking for how the error/state codes were found is because some of the mongodb tests are asserting something like
assertTrue(airbyteConnectionStatus.getMessage().contains("State code: -4"));
However from the standpoint of debugging, if someone wanted to look up what mongo db state code is how can it be determined that they're the right state codes? For instance looking up state codes for mongodb I could only find these state code but none of them match -4. It begs the question, how would these state codes be useful if documentation from mongodb doesn't match the codes in the test and what they mean?
| } | ||
|
|
||
| @Override | ||
| protected boolean compareDateTimeValues(String expectedValue, String actualValue) { |
There was a problem hiding this comment.
How is this work relevant to the error messages work?
There was a problem hiding this comment.
this logic is added here in the strict-encrypt connector from the main connector. Here is the number of the pull request and its name in which it was done - [15245] Destination-mysql: fixed normalization tests after changes in python part. The developer who added these changes only added them to the main connector, but forgot about strict-encrypt. Therefore, the tests will fail here, which interferes with testing and publishing.
...gres/src/test/java/io/airbyte/integrations/destination/postgres/PostgresDestinationTest.java
Outdated
Show resolved
Hide resolved
...source-snowflake/src/main/java/io.airbyte.integrations.source.snowflake/SnowflakeSource.java
Show resolved
Hide resolved
@ryankfu because of this part, i can't publish my changes to handle messages. This part breaks the tests and makes the normal publishing process impossible. (I try to adhere to the concept, and first run the tests there to publish without turning off the tests) |
|
/test connector=connectors/destination-redshift
Build PassedTest summary info: |
|
/test connector=connectors/destination-postgres
Build PassedTest summary info: |
…11322_error_message_1
|
/test connector=connectors/destination-mysql-strict-encrypt
Build PassedTest summary info: |
|
Work was completed in this ticket: #16202 |
Improve database sources and destinations error messages
When users are setting up a database connector and fail (due to misconfiguration) they are often provided a non-descriptive error message
The idea of this PR is to map common SqlState code to some human readable messages like: "Incorrect password or user name", "Incorrect host or port"
Thus user may have more information what might be wrong with provided configurations
Examples how it will look on UI:
-postgres fail username
snowflake fail role
oracle fail host
GCS fail bucket name
