-
Notifications
You must be signed in to change notification settings - Fork 208
Split API classes and interfaces #2
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
Changes from 1 commit
cd2d37e
749b2d3
1cfb4d2
bf4ec13
894051f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?php | ||
|
||
namespace MongoDB\Command; | ||
|
||
final class Command | ||
{ | ||
/** | ||
* @param array|object $document Command document | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to have this as both array and object again? |
||
*/ | ||
public function __construct($document) | ||
{ | ||
// ... | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
<?php | ||
|
||
namespace MongoDB\Command; | ||
|
||
// Note: consider combining implementation with \MongoDB\Query\QueryCursor | ||
final class CommandCursor implements \MongoDB\Cursor | ||
{ | ||
private $server; | ||
private $batchSize; | ||
private $cursorId; | ||
private $firstBatch; | ||
|
||
/** | ||
* @param Server $server | ||
* @param integer $cursorId | ||
* @param array $firstBatch | ||
*/ | ||
public function __construct(Server $server, $cursorId, array $firstBatch) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it make more sense to just pass in a document that was returned from the "runCommand" method (+ $server)? Is there a need to split it up into cursorId and firstBatch yourself? |
||
{ | ||
$this->server = $server; | ||
$this->cursorId = (integer) $cursorId; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly, you can't do this. On a 32bit platform (and Windows), $cursorID has to be a MongoInt64 object. |
||
$this->firstBatch = $firstBatch; | ||
} | ||
|
||
// Iterator methods... | ||
|
||
/** | ||
* @see \MongoDB\Cursor::getId() | ||
*/ | ||
public function getId() | ||
{ | ||
return $this->cursorId; | ||
} | ||
|
||
/** | ||
* @see \MongoDB\ServerResult::getServer() | ||
*/ | ||
public function getServer() | ||
{ | ||
return $this->server; | ||
} | ||
|
||
/** | ||
* @see \MongoDB\Cursor::setBatchSize() | ||
*/ | ||
public function setBatchSize($batchSize) | ||
{ | ||
$this->batchSize = (integer) $batchSize; | ||
} | ||
|
||
// Note: this expects consistent command response documents from the server | ||
static public function createFromCommandResult(CommandResult $result) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These factory methods may not even be necessary, as the userland layer can simply interpret the command response and construct the cursors outright. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, don't add this if it's not necessary... but, alternatively the constructor could just accept Server and CommandResult instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed in video chat, the constructor cannot take a CommandResult directly, since that leaves us with now way to create command cursors from parallelCollectionScan result documents. We agreed to remove this factory method completely. |
||
{ | ||
// extract $cursorId and $firstBatch from $result->getResponse() | ||
|
||
return new static($result->getServer(), $cursorId, $firstBatch); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
<?php | ||
|
||
namespace MongoDB\Command; | ||
|
||
interface CommandResult extends \MongoDB\ServerResult | ||
{ | ||
/** | ||
* @return array Original response document from the server | ||
*/ | ||
function getResponse(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably suggest calling this getResponseDocument() — it's more descriptive as to what it does. Right now, it could be interpreted as some pre-processed response. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
<?php | ||
|
||
namespace MongoDB; | ||
|
||
interface Cursor extends ServerResult, \Iterator | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is it extending ServerResult for? |
||
{ | ||
/** | ||
* @return integer Cursor identifier | ||
*/ | ||
function getId(); | ||
|
||
/** | ||
* @param integer $batchSize | ||
*/ | ||
function setBatchSize($batchSize); | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
<?php | ||
|
||
namespace MongoDB; | ||
|
||
final class Manager extends \SplObjectStorage | ||
{ | ||
/** | ||
* @param Server[] $servers | ||
* @param string $db Database name | ||
* @param array $options Connection options | ||
*/ | ||
public function __construct(array $servers, $db, $options) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bjori: What was the intention of the database and connection options here? If we have Server instances, aren't the connections already established? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed that both arguments should be removed, and we're be swapping the constructor with |
||
{ | ||
} | ||
|
||
static public function fromDSN($dsn) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this intended to be comparable to MongoClient's constructor? If so, wouldn't we need a DSN and connection options? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically the DSN should be able to include all connection options, but I would prefer it being a second (array) argument too. |
||
{ | ||
} | ||
|
||
final public function executeCommand($namespace, \MongoDB\Query $query, \MongoDB\ReadPreference $rp) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ReadPreference and WriteOptions classes need to be defined. Commands don't always require a ReadPreference, so I'm not sure how best to handle this. Additionally, commands and queries alike probably need general options to handle socket timeouts (something I imagine was intended to be handled with WriteOptions). Perhaps we need a ClientOptions argument, too? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
{ | ||
foreach($this->getConnectedServers() as $server) { | ||
if ($server->matchesReadPreference($rp)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is likely the place to apply the white list of commands that must be sent to a primary. |
||
return $server->executeQuery($namespace, $query); | ||
} | ||
} | ||
throw new NoServerMatchingReadPreference($rp); | ||
} | ||
|
||
final public function executeQuery($namespace, \MongoDB\Query $query, \MongoDB\ReadPreference $rp) | ||
{ | ||
foreach($this->getConnectedServers() as $server) { | ||
if ($server->matchesReadPreference($rp)) { | ||
return $server->executeQuery($namespace, $query); | ||
} | ||
} | ||
throw new NoServerMatchingReadPreference($rp); | ||
} | ||
|
||
final function executeWrite($namespace, \MongoDB\WriteBatch $batch, \MongoDB\WriteOptions $wo) | ||
{ | ||
foreach($this->getConnectedServers() as $server) { | ||
if ($server->isPrimary()) { | ||
return $server->executeWrite($namespace, $batch, $wo); | ||
} | ||
} | ||
|
||
throw new NoPrimaryAvailable; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NoPrimaryAvailable and NoServerMatchingReadPreference need to be defined. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
<?php | ||
|
||
namespace MongoDB\Query; | ||
|
||
final class Query | ||
{ | ||
/** | ||
* @param array|object $query Query document | ||
* @param array|object $selector Selector document | ||
* @param integer $flags Query flags | ||
* @param integer $limit Limit | ||
* @param integer $skip Skip | ||
*/ | ||
public function __construct($query, $selector, $flags, $skip, $limit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is essentially everything that makes up an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the case, we need to define the bitfields constants for all the flags. Or alternatively, we can make query flags an array with the options in there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed, I left |
||
{ | ||
// ... | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
<?php | ||
|
||
namespace MongoDB\Query; | ||
|
||
// Note: consider combining implementation with \MongoDB\Command\CommandCursor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both cursor classes can probably share most all of their code, especially if libmongoc is already abstracting command and query cursors for us. The "first batch" handling for a command cursor may still apply for query cursors, as it's comparable to the documents returned in the first There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's pretty much what firstBatch would mean for OP_QUERY results. |
||
final class QueryCursor implements \MongoDB\Cursor | ||
{ | ||
private $server; | ||
private $batchSize; | ||
private $cursorId; | ||
|
||
/** | ||
* @param Server $server | ||
* @param integer $cursorId | ||
*/ | ||
public function __construct(Server $server, $cursorId) | ||
{ | ||
$this->server = $server; | ||
$this->cursorId = (integer) $cursorId; | ||
} | ||
|
||
// Iterator methods... | ||
|
||
/** | ||
* @see \MongoDB\Cursor::getId() | ||
*/ | ||
public function getId() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do export this, it should always return a MongoInt64 object. |
||
{ | ||
return $this->cursorId; | ||
} | ||
|
||
/** | ||
* @see \MongoDB\ServerResult::getServer() | ||
*/ | ||
public function getServer() | ||
{ | ||
return $this->server; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this do? |
||
} | ||
|
||
/** | ||
* @see \MongoDB\Cursor::setBatchSize() | ||
*/ | ||
public function setBatchSize($batchSize) | ||
{ | ||
$this->batchSize = (integer) $batchSize; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
<?php | ||
|
||
namespace MongoDB\Query; | ||
|
||
interface QueryResult extends \MongoDB\ServerResult, \Traversable | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
namespace MongoDB; | ||
|
||
class Server | ||
{ | ||
const TYPE_MONGOS = 0x01; | ||
const TYPE_STANDALONE = 0x02; | ||
const TYPE_ARBITER = 0x03; | ||
const TYPE_SECONDARY = 0x04; | ||
const TYPE_PRIMARY = 0x05; | ||
|
||
/* These need to be final as they will not be called by the driver to | ||
* retrieve the information; these getters are only useful for userland. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bjori: Can you elaborate on this? Did you mean to imply that the extension is just going to read internal properties directly? |
||
final public function getHostname() {} | ||
final public function getPort() {} | ||
final public function getLatency() {} | ||
final public function getMaxWireVersion() {} | ||
final public function getMinWireVersion() {} | ||
final public function getServerType() {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we previous discussed just to have a "getServerInfo" method instead of all those getters? |
||
|
||
// extracting config info | ||
final public function isPassive() {} | ||
final public function isDelayed() {} | ||
|
||
final public function executeCommand($namespace, \MongoDB\Command\Command $command, \MongoDB\ReadPreference $rp) {} | ||
final public function executeQuery($namespace, \MongoDB\Query\Query $query, \MongoDB\ReadPreference $rp) {} | ||
final public function executeWrite($namespace, \MongoDB\Write\WriteBatch $batch, \MongoDB\WriteOptions $wo) {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this needs the insert, update and delete methods too. With $namespace being our MongoDB\Namespace value object. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
<?php | ||
|
||
namespace MongoDB; | ||
|
||
interface ServerError extends ServerResult | ||
{ | ||
/** | ||
* @return integer Server error code | ||
*/ | ||
function getCode(); | ||
|
||
/** | ||
* @return string Server error message | ||
*/ | ||
function getMessage(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is sometimes more than one message. wnote, jnote, msg, etc. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and Query do not have getter methods, since I assume their data will be accessed internally. Users shouldn't have any reason to read these objects after creating them.