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

Already on GitHub? Sign in to your account

Add support for storing role and meta data as json #8

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

alexanderkjeldaas commented Feb 21, 2013

No description provided.

Owner

nurpax commented Feb 21, 2013

Thanks! Storing these in JSON is a good idea :)

@alexanderkjeldaas This breaks the build though - you're missing a dependency on aeson in the .cabal file. You can see the build results here: https://travis-ci.org/nurpax/snaplet-sqlite-simple/builds/4955433 (see the above Travis error).

A good lower bound for aeson would be the latest shipped with the Haskell Platform.

I'd also suggest that you add a simple test case or too under the test/ directory.

I find that it's easiest to work on packages like this to use cabal-dev. This way you can run "cabal-dev install" at the top of the snaplet-sqlite-simple source tree and see that all the deps correctly build. To run tests, you need to run either "cabal-dev install --enable-tests" or "cabal-dev configure --enable-tests && cabal-dev build && cabal-dev test".

also, perhaps these might be better as TEXT instead of BLOB?

Contributor

alexanderkjeldaas commented Feb 21, 2013

I must have removed it while cleaning up the patch. Will fix.

On Thu, Feb 21, 2013 at 8:20 PM, Janne Hellsten notifications@github.comwrote:

Thanks! Storing these in JSON is a good idea :)

@alexanderkjeldaas https://github.com/alexanderkjeldaas This breaks the
build though - you're missing a dependency on aeson in the .cabal file. You
can see the build results here:
https://travis-ci.org/nurpax/snaplet-sqlite-simple/builds/4955433 (see
the above Travis error).

A good lower bound for aeson would be the latest shipped with the Haskell
Platform.

I'd also suggest that you add a simple test case or too under the test/
directory.

I find that it's easiest to work on packages like this to use cabal-dev.
This way you can run "cabal-dev install" at the top of the
snaplet-sqlite-simple source tree and see that all the deps correctly
build. To run tests, you need to run either "cabal-dev install
--enable-tests" or "cabal-dev configure --enable-tests && cabal-dev build
&& cabal-dev test".


Reply to this email directly or view it on GitHubhttps://github.com/nurpax/snaplet-sqlite-simple/pull/8#issuecomment-13907279.

Contributor

alexanderkjeldaas commented Feb 21, 2013

Pushed. Haskell platform doesn't ship aeson.

On Thu, Feb 21, 2013 at 11:21 PM, Alexander Kjeldaas <
alexander.kjeldaas@gmail.com> wrote:

I must have removed it while cleaning up the patch. Will fix.

On Thu, Feb 21, 2013 at 8:20 PM, Janne Hellsten notifications@github.comwrote:

Thanks! Storing these in JSON is a good idea :)

@alexanderkjeldaas https://github.com/alexanderkjeldaas This breaks
the build though - you're missing a dependency on aeson in the .cabal file.
You can see the build results here:
https://travis-ci.org/nurpax/snaplet-sqlite-simple/builds/4955433 (see
the above Travis error).

A good lower bound for aeson would be the latest shipped with the Haskell
Platform.

I'd also suggest that you add a simple test case or too under the test/
directory.

I find that it's easiest to work on packages like this to use cabal-dev.
This way you can run "cabal-dev install" at the top of the
snaplet-sqlite-simple source tree and see that all the deps correctly
build. To run tests, you need to run either "cabal-dev install
--enable-tests" or "cabal-dev configure --enable-tests && cabal-dev build
&& cabal-dev test".


Reply to this email directly or view it on GitHubhttps://github.com/nurpax/snaplet-sqlite-simple/pull/8#issuecomment-13907279.

Contributor

alexanderkjeldaas commented Feb 21, 2013

Hold on, I have messed things up with trying to clean up the patch.

On Thu, Feb 21, 2013 at 11:23 PM, Alexander Kjeldaas <
alexander.kjeldaas@gmail.com> wrote:

Pushed. Haskell platform doesn't ship aeson.

On Thu, Feb 21, 2013 at 11:21 PM, Alexander Kjeldaas <
alexander.kjeldaas@gmail.com> wrote:

I must have removed it while cleaning up the patch. Will fix.

On Thu, Feb 21, 2013 at 8:20 PM, Janne Hellsten <notifications@github.com

wrote:

Thanks! Storing these in JSON is a good idea :)

@alexanderkjeldaas https://github.com/alexanderkjeldaas This breaks
the build though - you're missing a dependency on aeson in the .cabal file.
You can see the build results here:
https://travis-ci.org/nurpax/snaplet-sqlite-simple/builds/4955433 (see
the above Travis error).

A good lower bound for aeson would be the latest shipped with the
Haskell Platform.

I'd also suggest that you add a simple test case or too under the test/
directory.

I find that it's easiest to work on packages like this to use cabal-dev.
This way you can run "cabal-dev install" at the top of the
snaplet-sqlite-simple source tree and see that all the deps correctly
build. To run tests, you need to run either "cabal-dev install
--enable-tests" or "cabal-dev configure --enable-tests && cabal-dev build
&& cabal-dev test".


Reply to this email directly or view it on GitHubhttps://github.com/nurpax/snaplet-sqlite-simple/pull/8#issuecomment-13907279.

alexanderkjeldaas added some commits Feb 21, 2013

@alexanderkjeldaas alexanderkjeldaas Make tests pass.
The test suite does some invalid reads of non-upgraded
databases.  This exposed the fact that SQLite cannot
decode a NULL-blob into an empty ByteString.
So I have added "blob not null default x''" as
the type for roles and meta.

Some code was also left out from the last commit.
4954ba3
@alexanderkjeldaas alexanderkjeldaas Updated tests to check roles/meta. Added destroyUser test. 2f7004d
Contributor

alexanderkjeldaas commented Feb 22, 2013

I've pushed something that passes the tests now, added asserts for
roles/meta, and added a completely unrelated destroyUser test.

On Thu, Feb 21, 2013 at 11:30 PM, Alexander Kjeldaas <
alexander.kjeldaas@gmail.com> wrote:

Hold on, I have messed things up with trying to clean up the patch.

On Thu, Feb 21, 2013 at 11:23 PM, Alexander Kjeldaas <
alexander.kjeldaas@gmail.com> wrote:

Pushed. Haskell platform doesn't ship aeson.

On Thu, Feb 21, 2013 at 11:21 PM, Alexander Kjeldaas <
alexander.kjeldaas@gmail.com> wrote:

I must have removed it while cleaning up the patch. Will fix.

On Thu, Feb 21, 2013 at 8:20 PM, Janne Hellsten <
notifications@github.com> wrote:

Thanks! Storing these in JSON is a good idea :)

@alexanderkjeldaas https://github.com/alexanderkjeldaas This breaks
the build though - you're missing a dependency on aeson in the .cabal file.
You can see the build results here:
https://travis-ci.org/nurpax/snaplet-sqlite-simple/builds/4955433 (see
the above Travis error).

A good lower bound for aeson would be the latest shipped with the
Haskell Platform.

I'd also suggest that you add a simple test case or too under the test/
directory.

I find that it's easiest to work on packages like this to use
cabal-dev. This way you can run "cabal-dev install" at the top of the
snaplet-sqlite-simple source tree and see that all the deps correctly
build. To run tests, you need to run either "cabal-dev install
--enable-tests" or "cabal-dev configure --enable-tests && cabal-dev build
&& cabal-dev test".


Reply to this email directly or view it on GitHubhttps://github.com/nurpax/snaplet-sqlite-simple/pull/8#issuecomment-13907279.

Owner

nurpax commented Feb 22, 2013

The build is still not passing (see the Travis details from https://travis-ci.org/nurpax/snaplet-sqlite-simple/builds/4971359) - but with a different error though.

The unit tests for the new meta & role store do not test whether any data actually gets persisted. It'd be better if you added some roles for a user, saved it and then checked whether that data correctly came back from the database. Just asserting against [] would pass even in previous snaplet-sqlite-simple versions that did not support persisting these fields.

Owner

nurpax commented Feb 22, 2013

Also in alexanderkjeldaas/snaplet-sqlite-simple@4954ba3:

The test suite does some invalid reads of non-upgraded
databases. This exposed the fact that SQLite cannot
decode a NULL-blob into an empty ByteString.

Tests should always run using the most recent schema version. If some tests are running on old schema and fail, then we should fix the tests to not do that and ensure the upgrades happen the right way.

Sqlite-simple is always able to deal with NULLs if the types are right. If you query to a destination type of Maybe [ByteString], a NULL will be converted to Nothing.

Owner

nurpax commented Feb 22, 2013

I'll fix this myself, will send you a proposal soon. The ByteString.fromStrict problem is because bytestring in Haskell Platform doesn't have 0.10 yet.

Contributor

alexanderkjeldaas commented Feb 22, 2013

With the updated schema Sqlite should always decode properly.

Good if you fix this, I don't have the haskell platform installed currently.

I know I haven't added any real tests. I am just trying to understand the test setup...

Owner

nurpax commented Feb 22, 2013

I uploaded a cleaned up version of your pull request on https://github.com/nurpax/snaplet-sqlite-simple/commits/alex-pr-fixes. I rewrote the history so that I squashed all the incremental fixes into a single change - no need to retain them in the history as some of them even break the build.

I know the new code is a bit longer than the original.. But I wanted to store roles / meta as NULLs if they're empty. This also makes the schema default empty string issue go away.

You should be able to update this pull request by git checkouting 4d92cdb, and "git push -f"'ing it to your fork. WARNING your latest changes will be overwritten - perhaps stick those into a separate pull request instead?

Note that I cheated and made fixes to sqlite-simple too. :) It was missing an instance for Data.Text.Lazy strings.

Contributor

alexanderkjeldaas commented Feb 22, 2013

Closing this as I've opened another pull request with your changes. But note that the tests fail.

Owner

nurpax commented Feb 22, 2013

@alexanderkjeldaas I'll try with your tests this evening - I didn't get a chance to test my changes yet other than checking they compiled. Looks like your tests will come in handy :)

Contributor

alexanderkjeldaas commented Feb 22, 2013

Aeson.decode' fails with the given input

This is what you'll see in the tests with the change below:
a"["Superman","Journalist"]"
bNothing
a"["Superman","Journalist"]"
bNothing
a"["Superman","Journalist"]"
bNothing
a"["Superman","Journalist"]"
bNothing

    traceAnItem :: Show a => String -> a -> a
    traceAnItem s x = trace (s ++ (show x)) x

    decodeRoles = do
      roles <- fmap (fmap (maybeToList . (traceAnItem "b") . A.decode'

. (traceAnItem "a") . LT.encodeUtf8)) _userRoles
return $ fromMaybe [] roles

Something similar works in ghci

*Main Data.Text.Lazy Data.Text.Lazy.Encoding Data.Aeson Data.ByteString>
(decode $ encodeUtf8 $ Data.Text.Lazy.pack "["Superman","Journalist"]")
:: Maybe [ByteString]
Just ["Superman","Journalist"]

I'm guessing that you'll figure this out a lot easier than me, so I'll wait
for your move.

Alexander

On Fri, Feb 22, 2013 at 12:17 PM, Janne Hellsten
notifications@github.comwrote:

@alexanderkjeldaas https://github.com/alexanderkjeldaas I'll try with
your tests this evening - I didn't get a chance to test my changes yet
other than checking they compiled. Looks like your tests will come in handy
:)


Reply to this email directly or view it on GitHubhttps://github.com/nurpax/snaplet-sqlite-simple/pull/8#issuecomment-13938739.

Owner

nurpax commented Feb 22, 2013

@alexanderkjeldaas Thanks, your ghci session was very helpful. There was a bit too much type (class) magic going on in my JSON decode part. Made it a bit more straightforward now in 42abdc6.

I'll release a new version of sqlite-simple while at it - that's needed for this stuff to compile.

Owner

nurpax commented Feb 22, 2013

@alexanderkjeldaas It's ready to be uploaded to Hackage. I didn't do that yet just in case you had some other ideas or tweaks you wanted to do before releasing it. Perhaps I'll nail down #7 too before the next release.

Contributor

alexanderkjeldaas commented Feb 22, 2013

Ship it!

On Fri, Feb 22, 2013 at 9:36 PM, Janne Hellsten notifications@github.comwrote:

@alexanderkjeldaas https://github.com/alexanderkjeldaas It's ready to
be uploaded to Hackage. I didn't do that yet just in case you had some
other ideas or tweaks you wanted to do before releasing it. Perhaps I'll
nail down #7 https://github.com/nurpax/snaplet-sqlite-simple/issues/7too before the next release.


Reply to this email directly or view it on GitHubhttps://github.com/nurpax/snaplet-sqlite-simple/pull/8#issuecomment-13969934.

Owner

nurpax commented Feb 23, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment