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

add storage CassandraCQL #642

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

tobias74
Copy link
Contributor

the existing Cassandra Storage uses a library that is conflicting with the php-cassandra-driver (datastax) I am using. For exercise and fun I adapted the existing Cassandra-Stroage to work with CQL and the datastax-php-driver.

Maybe someone finds this usefull :-)

Tobias

@tobias74
Copy link
Contributor Author

since I have copypasted lots of code from the original cassandra storage, maybe there should be a common base-class for both storages.

I did not want to be too intrusive, so I left existing code unchanged and just added the new storage.

@bshaffer
Copy link
Owner

Thanks Tobias! I'm ok with not having a common base class. However we do
need to add your Storage Adapter to BaseTest and Test/Bootstrap so it can
get tested with the rest of the test suite.
On Thu, Sep 17, 2015 at 5:04 AM Tobias Gassmann [email protected]
wrote:

since I have copypasted lots of code from the original cassandra storage,
maybe there should be a common base-class for both storages.

I did not want to be too intrusive, so I left existing code unchanged and
just added the new storage.


Reply to this email directly or view it on GitHub
#642 (comment)
.

@bshaffer
Copy link
Owner

@tobias74 any word on this? I can provide you with more information on how to add these tests.

@tobias74
Copy link
Contributor Author

I will add the tests this weekend, I have to get up to speed with Travis-CI. :-)
How could I run the tests locally?

@bshaffer
Copy link
Owner

Just pull the repo, run composer install, and then run phpunit... you should be good to go.

It'll skip tests automatically it can't connect to, but if you run phpunit -v you'll see the output for how to set those up.

@dylwylie
Copy link

This is exactly what I was about to start writing! Thanks @tobias74

@bshaffer
Copy link
Owner

bshaffer commented Oct 6, 2015

@tobias74 any word on this? Will you add this to the tests here?

@bshaffer
Copy link
Owner

bshaffer commented Oct 6, 2015

You will also need to add more information about the dependent Cassandra client library, and also type-hinting in your constructor.

@bshaffer
Copy link
Owner

bshaffer commented Oct 6, 2015

To answer your question about testing, just pull down the library and run phpunit in the project root. It'll skip the other storage tests unless you have them running, which is fine because you don't need to test those. Just model your tests after the existing cassandra storage class

@tobias74
Copy link
Contributor Author

tobias74 commented Oct 7, 2015

I have added the tests according to the existing Cassandra-Storage. Three of my tests are still failing when I run phpunit locally:

  1. Warning
    The data provider specified for OAuth2\OpenID\Storage\AuthorizationCodeTest::testCreateAuthorizationCode is invalid.
    line 1:173 extraneous input ')' expecting EOF

  2. OAuth2\OpenID\Storage\UserClaimsTest::testGetUserClaims with data set add PDO storage support for refresh tokens #9 (OAuth2\Storage\CassandraCQL Object (...))
    Failed asserting that '[email protected]' matches expected null.

  3. OAuth2\Storage\PublicKeyTest::testSetAccessToken with data set add PDO storage support for refresh tokens #9 (OAuth2\Storage\CassandraCQL Object (...))
    Failed asserting that '-----BEGIN CERTIFICATE-----
    MIICiDCCAfGgAwIBAgIBADANBgkqhkiG9w0BAQQFADA9MQswCQYDVQQGEwJVUzEL
    ...
    FRrlM1f6s9VTLWvwGItjfrof0Ba8Uq7ZDSb9Xg==
    -----END CERTIFICATE-----' matches expected null.

I am still looking into that, maybe someone could point me in the right direction?
Thank you :-)

@bshaffer
Copy link
Owner

bshaffer commented Oct 7, 2015

These tests are passing for the existing OAuth2\Storage\Cassandra implementation, so most likely you'll need to debug your added storage class to make the tests pass. If you crack open the tests themselves, you'll see where the assertions are being made.

The first error (extraneous input ')' expecting EOF) seems like you have a syntax error in your CQL.
The second and third error seem to imply your Cassandra test data has not been set up correctly (see OAuth2\Storage\Bootstrap::createCassandraDB)

Good luck!

@bshaffer
Copy link
Owner

bshaffer commented Oct 7, 2015

So a question to help my own cassandra understanding - what are the advantages / disadvantages to the two cassandra libraries being used? Is one considered deprecated, or are they both viable options? Do the names Cassandra and CassandraCQL properly describe the differences between them?

@tobias74
Copy link
Contributor Author

tobias74 commented Oct 7, 2015

Yes, I had an error in my CQL! Now the tests seem to pass:


OK, but incomplete, skipped, or risky tests!
Tests: 407, Assertions: 1197, Skipped: 126.


Concerning advantages and disadvantages: I don't know :-)
I had to choose a library in order to get started with cassandra. So I went with the datastax-library because it came up first on google and the given examples used CQL-Queries.

One downside of the datastax-library is the somewhat involved setup-process ("pecl install cassandra" throws errors on my machine, so I had to build from source manually).

The name "CassandraCQL" is just a working title, I am open to suggestions :-)

@dylwylie
Copy link

dylwylie commented Oct 7, 2015

To add, phpcassa is deprecated and the author recommends moving to Datastax.

Also @tobias74, I noticed you haven't made us of Cassandra's row TTL to have tokens automatically expire. The $expire variable passed into setValue() is ignored. I wrote an example quickly if you'd like to look -> dylwylie@b583ae8

@tobias74
Copy link
Contributor Author

tobias74 commented Oct 8, 2015

Thanks @Dylan1312 :-)
should I just copy your code or do you want to issue a pull-request?

@dylwylie
Copy link

dylwylie commented Oct 8, 2015

Go ahead and copy if you're happy with it 👍

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.

3 participants