GridFS 2015 #100
GridFS 2015 #100
Conversation
| @@ -191,4 +191,19 @@ | |||
| */ | |||
| void createCollection(String collectionName, CreateCollectionOptions createCollectionOptions); | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I don't think that MongoDatabase should be a factory for GridFSBucket instances. GridFS is an application on top of MongoDB, and therefore GridFSBucket should depend on MongoDatabase, not the other way around.
The advantage of this approach is that we don't have to expose a concrete implementation of GridFSBucket, but I still think it's a mistake.
How about
public class GridFSBuckets {
public static GridFSBucket create(MongoDatabase);
}
There was a problem hiding this comment.
Nice - that also means I can move all the code under com.mongodb.client.gridfs
| * @return the GridFS find iterable interface | ||
| * @mongodb.driver.manual tutorial/query-documents/ Find | ||
| */ | ||
| GridFSFindIterable find(); |
There was a problem hiding this comment.
In the CRUD spec we specifically decided that filters are mandatory.
Why this overload here?
There was a problem hiding this comment.
This is inline with MongoCollection. A {} filter will be used underneath.
Done.
There was a problem hiding this comment.
So MongoCollection doesn't follow the CRUD spec?
There was a problem hiding this comment.
It follows a version of it - when it was allowed ;)
There was a problem hiding this comment.
Perhaps its more appropriate to say it is a superset of the CRUD spec.
There was a problem hiding this comment.
Done. (no action as is consistent with what we have).
| if (fileInfo == null) { | ||
| throw new MongoGridFSException(format("No file found with the id: %s", id)); | ||
| } | ||
| return new GridFSDownloadStreamImpl(fileInfo, chunksCollection); |
There was a problem hiding this comment.
Does the constructor open the stream? (If so, is it a good idea to throw exceptions from constructors?)
Or do we need to call open on the stream before returning it?
There was a problem hiding this comment.
There is no stream is opened as such - this just initiates a class that extends the abstract InputStream class.
There was a problem hiding this comment.
InputStream has no open method - http://docs.oracle.com/javase/7/docs/api/index.html?java/io/InputStream.html
Done.
There was a problem hiding this comment.
There is no open method on Java streams. They are implicitly open on construction.
Use the users codec registry with the default for the files collection Use the default codec registry for the chunks collection
JAVA-1713
|
@jyemin updated based on comments, also merged in the docs. Regarding the abort() clean ups - this can be done with JAVA-1920 - I've copied in your comment to the ticket, so the context isn't lost. |
|
This got a bit long and sat for a while due to my absence, but I don't see any more open issues. If you can confirm that, then LGTM. |
| if (buffer != null) { | ||
| chunkToCheck += 1; | ||
| } | ||
| Document chunk = getChunk(chunkToCheck); |
There was a problem hiding this comment.
I noticed this bit of code (lines 87 - 90) while reviewing #109. Can you explain the intention?
There was a problem hiding this comment.
Yes, at EOF we must validate there are no extra chunks OR that if there is an extra chunk it is empty. Prior to the change we didnt check but that was in breach of the spec.
|
Rebased, Squashed and merged |
Had to update codenarc as it couldn't parse the GridFS Smoke test.
As the yml tests are being refactored I paused work on that until they have been updated.