Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

statement.split(";") cannot be used with strings which contains symbo… #24

Merged
merged 14 commits into from Aug 5, 2018

Conversation

@urvanov-ru
Copy link
Contributor

commented Mar 27, 2018

…l ";" like "vasya;petya". I think, it would be better to check for the end of line after ";" symbol. At least we would have an opportunity to use semicolons in text.

Example for test:
https://github.com/urvanov-ru/cassandra-maven-plugin-semicolon-test

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@olamy ?

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2018

@giejay

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

Looks good, but unfortunately I do not have the access rights to merge it

@djarnis73

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

This is not a fix in my opinion, it will quickly break with a cryptic error message (like today), I have a version that uses the cql lexer, so it should handle any wellformed cql. But I did not have time to polish it into a PR.

The splitter code looks like this:

protected static String[] splitStatementsUsingCqlLexer(String statements) {
    ANTLRStringStream stream = new ANTLRStringStream(statements);
    CqlLexer lexer = new CqlLexer(stream);
    ArrayList<String> statementList = new ArrayList<String>();
    StringBuffer currentStatement = new StringBuffer();
    boolean inComment;
    // Not the prettiest code i ever wrote, but it gets the job done.
    for (Token token = lexer.nextToken(); token.getType() != Token.EOF; token = lexer.nextToken()) {
        if (token.getText().equals(";")) {
            // when we meet a ; terminate current statement and prepare the next
            currentStatement.append(";");
            statementList.add(currentStatement.toString());
            currentStatement = new StringBuffer();
        } else if (token.getType() == CqlLexer.STRING_LITERAL) {
            // If we meet a string we should quote it and escape any enclosed ' as ''
            currentStatement.append("'");
            // TODO: There must be a cassandra util method somewhere that escapes a string for sql
            currentStatement.append(token.getText().replaceAll("'", "''"));
            currentStatement.append("'");
        } else {
            currentStatement.append(token.getText());
        }
    }
    if (currentStatement.length() > 0 && currentStatement.toString().trim().length() > 0) {
        statementList.add(currentStatement.toString());
    }
    return statementList.toArray(new String[statementList.size()]);
}
@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2018

Thanks. I will try to update my pull request with your code.

Add useCqlLexer option
This will use the cql lexer instead of blindly splitting on ;
@djarnis73

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

I have pushed my changes to my clone, you can see full commit here: djarnis73@0ed68b2 i can make a PR for it if it looks good to you.

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

@djarnis73 Your code much better but it is not finished yet. It does not work with my cql-code from my job for which I trying to adapt the plugin. And it still has problems with comments. I will try to fix it. I will make a pull request when I finish.

urvanov-ru added 3 commits Apr 30, 2018
Merge branch 'feature/parse_using_cql_lexer' into cql-split-fix
Conflicts:
	src/main/java/org/codehaus/mojo/cassandra/AbstractCqlExecMojo.java
@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2018

How about now?

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2018

@stephenc @seblm @ajs6f @giejay @djarnis73 @Willem1987 @dmitrymurashenkov @olamy
Can anybody look this pull request and merge if it is ok?

* It is not enabled by default since has not been extensively tested.
*
* @parameter default-value=false
* @since 3.6

This comment has been minimized.

Copy link
@djarnis73

djarnis73 May 8, 2018

Contributor

since should be bumped to 3.7, since 3.6 is already released.

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2018

Fixed.

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2018

@stephenc @seblm @ajs6f @giejay @djarnis73 @Willem1987 @dmitrymurashenkov @olamy
Are there any guys with write permissions? Who can merge?

@ajs6f

This comment has been minimized.

Copy link
Contributor

commented May 17, 2018

I don't know why you think I can merge your work. I have no role in this project and have never contributed to it.

@urvanov-ru

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2018

I think It is better to fork this repo and produce my own version of this plugin. I have really no idea whom and how should I push to force him to merge this branch.

@olamy olamy merged commit f654098 into mojohaus:master Aug 5, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.