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

Adds support for statically typed insert statements. #133

Closed
wants to merge 2 commits into from
Closed

Adds support for statically typed insert statements. #133

wants to merge 2 commits into from

Conversation

MicahZoltu
Copy link

See the new test in PreparedStatementsTest for an example of the problem being fixed. Previously the test would fail on the call to setLong because the prepared statement result from the stub server did not contain column type information.

Fixes #132

This is not ready for merge, but I am a bit stuck. The original bug has been fixed, but some tests are now failing and I don't fully understand why (nor do I understand what they were testing). In particular, it appears the column_types were working for select statements and now they aren't. By not sending back column information in the prepare result (just commenting out the columns.forEach) fixes the old tests... but of course breaks the new test.

See the new test in `PreparedStatementsTest` for an example of the problem being fixed.  Previously the test would fail on the call to `setLong` because the prepared statement result from the stub server did not contain column type information.
@MicahZoltu
Copy link
Author

I have fixed the failing tests by changing the PreparedResult to include the columns only if there are no primed variableTypes.

This seems pretty terrible to me, but I'm beginning to suspect fully supporting priming columns in both ways together would require some architectural changes or at the least a breaking change in the API. If anyone has any better ideas on how to accomplish this please let me know.

@MicahZoltu
Copy link
Author

I have added support for asserting on activity. I am still very unhappy with this solution, but it gets the job done and fixes my use case. I would love to see a better solution, but I really do need something that works more than I need clean code at this juncture. :/

@MicahZoltu
Copy link
Author

Bleh. My fix for asserting on activity doesn't actually work because the map.values() doesn't give the results in a well defined order, so you can't reliably assert on it. :(

…mn metadata.

I am not very happy with the solution, but I lack the context for the overall architecture of this project to fix this problem in a better way.  Right now I suspect that things would fall apart if you tried to use both the column metadata and variable types.
@MicahZoltu
Copy link
Author

I am now using this branch for my Cassandra testing since, even with the caveats, it is better than nothing. What are the chances of this getting merged into mainline? If they are low, then I'll look into making my fork a bit more permanent (setup CI, etc.). If they are high I'll continue waiting.

@chbatey
Copy link
Member

chbatey commented Feb 16, 2016

thanks for your work on this @Zoltu I should have time to look at this properly at the weekend

@chbatey
Copy link
Member

chbatey commented Aug 3, 2016

Closing this per the comment on the issue

@chbatey chbatey closed this Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ColumnMetadata.column does not appear to do anything.
2 participants