Database-based Argument Setter #77
Conversation
|
|
||
| @Override | ||
| public void configurePipeline(PipelineConfigurer pipelineConfigurer) | ||
| throws IllegalArgumentException { |
There was a problem hiding this comment.
@CuriousVini fixed
| } catch (Exception e) { | ||
| collector.addFailure(e.getMessage(), null).withStacktrace(e.getStackTrace()); | ||
| } | ||
| config.validate(collector); |
There was a problem hiding this comment.
why do we need to call config.validate(collector) 2 times?
There was a problem hiding this comment.
@CuriousVini removed redundant call
| collector.addFailure("Invalid connection string", "Connection string cannot be empty"); | ||
| } | ||
| if (!containsMacro(ConnectionConfig.USER) && Strings.isNullOrEmpty(this.user)) { | ||
| collector.addFailure("Invalid username", "Username cannot be empty"); |
There was a problem hiding this comment.
please add period at the end of every failure message and its cause
There was a problem hiding this comment.
@CuriousVini updated all messages
|
|
||
| collector.getOrThrowException(); | ||
| } | ||
|
|
There was a problem hiding this comment.
@CuriousVini fixed
| } | ||
| if (resultSet.next()) { | ||
| failureCollector | ||
| .addFailure("More than one record found", |
There was a problem hiding this comment.
More than one records found.
There was a problem hiding this comment.
@CuriousVini fixed
| if (resultSet.next()) { | ||
| failureCollector | ||
| .addFailure("More than one record found", | ||
| "The argument selection conditions return multiple rows"); |
There was a problem hiding this comment.
what is the expected corrective action that needs to be taken from user?
There was a problem hiding this comment.
@CuriousVini updated
| ResultSet resultSet = statement.executeQuery(config.getQuery()); | ||
| boolean hasRecord = resultSet.next(); | ||
| if (!hasRecord) { | ||
| failureCollector.addFailure("Ne record found", |
There was a problem hiding this comment.
Typo: No records found.
Please add period to every failure message and corresponding corrective action
There was a problem hiding this comment.
@CuriousVini updated messages to have corrective action.
| boolean hasRecord = resultSet.next(); | ||
| if (!hasRecord) { | ||
| failureCollector.addFailure("Ne record found", | ||
| "No data is returned for the argument selection conditions"); |
There was a problem hiding this comment.
If there are no records should we return from here and not do any further processing?
There was a problem hiding this comment.
@CuriousVini fixed to return and not do further processing.
| .getConnection(config.getConnectionString(), connectionProperties); | ||
| Statement statement = connection.createStatement(); | ||
| ResultSet resultSet = statement.executeQuery(config.getQuery()); | ||
| boolean hasRecord = resultSet.next(); |
There was a problem hiding this comment.
does resultset has - hasNext() method? we should use that method.
There was a problem hiding this comment.
@CuriousVini resulset does not have hasNext() instead it has next() which moves the cursor to next record and returns true/false based on whether record is available or not.
| "The argument selection conditions must match only one record."); | ||
| } | ||
| } catch (SQLException e) { | ||
| throw new SQLException(e.getMessage(), e.getSQLState(), e.getErrorCode()); |
There was a problem hiding this comment.
why do we catch the sqlexception and throw the same exception? It should throw same SQLException
There was a problem hiding this comment.
a few comments. Fix and squash the commits
@CuriousVini done.
| collector.addFailure("Invalid password.", "Password cannot be empty."); | ||
| } | ||
| if (!containsMacro(DATABASE_NAME) && Strings.isNullOrEmpty(this.getDatabaseName())) { | ||
| collector.addFailure("Invalid database.", "Invalid database is specified."); |
There was a problem hiding this comment.
Invalid database is specified. -> Invalid database must be specified.
| collector.addFailure("Invalid database.", "Invalid database is specified."); | ||
| } | ||
| if (!containsMacro(TABLE_NAME) && Strings.isNullOrEmpty(this.getTableName())) { | ||
| collector.addFailure("Invalid table.", "Invalid table is specified."); |
ailegion
force-pushed
the
feature/db-argument-setter
branch
from
a6d75ad to
4d1eff1
Compare
added: Database-based Argument Setter support
JIRA: https://issues.cask.co/browse/PLUGIN-173