search

sky-uk / cqlmigrate

Cassandra schema migration library
BSD 3-Clause "New" or "Revised" License
47 stars 29 forks source link

`CqlPaths` file operation doesn't appear to support files embedded in a JAR #15

Closed oliverlockwood closed 8 years ago

oliverlockwood commented 8 years ago

Trying to run from within a jar on files contained within that jar, the following exception is hit.

[2015-12-17 17:04:55,095] umv-service INFO [main] u.s.c.CqlMigratorImpl - [request=] [client=] [x-skyint-requestid=] [] [] [] [] Loading cql files from [cqlschema/common, cqlschema/local] Exception in thread "main" java.lang.UnsupportedOperationException at com.sun.nio.zipfs.ZipPath.toFile(ZipPath.java:597) at uk.sky.cqlmigrate.CqlPaths.lambda$create$4(CqlPaths.java:28) at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:193) at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1374) at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:481) at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:471) at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:151) at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:174) at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:418) at uk.sky.cqlmigrate.CqlPaths.create(CqlPaths.java:32) at uk.sky.cqlmigrate.CqlMigratorImpl.migrate(CqlMigratorImpl.java:83) at uk.sky.cqlmigrate.CqlMigratorImpl.migrate(CqlMigratorImpl.java:65)

I gather from @adamdougal that there's been a fair amount of refactoring from the legacy CqlMigrate to update to Java 8 - and I'm guessing that thus far this (mainline) use case has not been tested?

This suggests to me that we probably need the following:

oliverlockwood commented 8 years ago

Update on this: @kevinpotgieter and I have written a fix, but we've paused it until we have the integration tests set up to prove it, which is what we're working on now.

jsravn commented 8 years ago

Yup looks like a regression. If I recall correctly, in the original one we had the same issue with toFile and fixed it there. That's what we get for not writing a test for it.

jsravn commented 8 years ago

I don't really get why the directory stream stuff was ripped out and replaced with old school File operations. But anyways, that's what broke it.

oliverlockwood commented 8 years ago

@jsravn as per the PR I've raised (https://github.com/sky-uk/cqlmigrate/pull/16), it seems non-trivial to test... unless you've got a better suggestion on how to test it?

jsravn commented 8 years ago

It's not straightforward that's for sure. Maybe bundle a jar in test resources and load it with the classloader.

balooo commented 8 years ago

Fixed by https://github.com/sky-uk/cqlmigrate/pull/16