Skip to content

Split and modify CRUD examples #1

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 13, 2014
Merged

Split and modify CRUD examples #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
594dbdb
Split and modify CRUD examples
jmikola May 13, 2014
8a00ff2
Use value objects and make QueryResult IteratorAggregate
jmikola May 13, 2014
eee840c
Query constructor should take query object, not just criteria
jmikola May 13, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 0 additions & 136 deletions docs/examples/crud.php
View file
Open in desktop

This file was deleted.

27 changes: 27 additions & 0 deletions docs/examples/insert.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<?php

$batch = new \MongoDB\Batch\InsertBatch();
$batch
->add($hannes)
->add($hayley);

$writeOptions = array(
"ordered" => true,
"writeConcern" => array(
"w" => 3,
"wtimeout" => 42,
),
);

$mm = new \MongoDB\Manager("mongodb://server1,server2/?replicaSet=FOOBAR");

$results = $mm->executeWrite("db.collection", $batch, $writeOptions);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than InsertBatch telling us that the operations are inserts (by its class name), I don't see why we shouldn't just pass in an array of operations. This would mean we'd have specific methods like executeInsert(), which I think is fine. In the C code, it means there's no need to check the class and then branch off internally to the appropriate write method in libmongoc. Also, being able to pass an array of operations (e.g. documents to insert or the update arguments) in one shot addresses @derickr's concern about having a simple interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a classname makes it easier and cleaner to validate the actual operations.
Having to validate enormous array all the time is both complex and not something you want to have multiple branches of in one method.

Allow individual classes to provide their own validation and then simply be typehinted simplifies the entire chain of events and allows us to keep small pieces of selfcontained code.

Having an executeInsert(array($document1, $document2)); is begging for validation issues and "whops, I did executeInsert($document)"

Providing a \MongoDB\Manager\CRUD->insertOne($document); is a trivial wrapper though we could also provide as part of the extension as a simplified helper API....


assert($results instanceof \MongoDB\WriteResults);

printf(
"Inserted %d documents %s in %d msec\n",
$results->getOpCount(),
$results->getServer()->getHostname(),
$results->getTimer()
);
62 changes: 62 additions & 0 deletions docs/examples/model.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

class Person implements BsonSerializable {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed Mongolizable to BsonSerializable. This mimics JsonSerializable and I don't see the harm in referring to BSON if we're already going to do it in the type mappings elsewhere.

protected $name;
protected $nick;
protected $addresses = array();

function __construct($name, $nick, array $addresses) {
$this->name = $name;
$this->nick = $nick;
foreach($addresses as $address) {
if ($address instanceof Address) {
$this->addresses[] = $address;
}
}
}

function getAddresses() {
return $this->addresses;
}
}

class Address implements BsonSerializable {
protected $address;
protected $country;

function __construct($address, Country $country) {
$this->address = $address;
$this->country = $country;
}

function getCountry() {
return $this->country;
}
}

class Country implements BsonSerializable {
protected $name;

function __construct($name) {
$this->name;
}

function getName() {
return $name;
}
}

$norway = new Country("Norway");
$iceland = new Country("Iceland");
$usa = new Country("USA");

$menlo = new Address("22 Coleman Pl", $usa);

$addresses = array(
new Address("Dynekilgata 15", $norway),
new Address("Mánabraut 4", $iceland),
$menlo,
);

$hannes = new Person("Hannes", "bjori", $addresses);
$hayley = new Person("Hayley", "Ninja", array($menlo));
41 changes: 41 additions & 0 deletions docs/examples/query.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?php

$queryDocument = array(
'$query' => array("name" => "Hannes"),
'$orderBy' => array('name' => 1),
'$comment' => 'More special stuff',
);

$queryFields = array(
'fieldname' => 1,
'fieldname.two' => 1,
);

$flags = EXHAUST | NO_CURSOR_TIMEOUT;
$limit = -3; // Kill server cursor after 3 documents
$skip = 0; // Don't skip any documents

$query = new \MongoDB\Query($queryDocument, $queryFields, $flags, $limit, $skip);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, this could be a single argument with an array of our own concept query options:

  • query document (where we store query modifiers)
  • selector document
  • flags
  • limit
  • skip

I'm against the original design, which had the selector, flags, limit, and skip options within the query document, as that requires C code to pluck out options before we can send anything to libmongoc. Also, there is a hypothetical situation if the server added a $limit query modifier which was different than the current concept of a limit. Since the server does document valid query modifiers, I don't think we should attempt to put other fields inside that document at this time.

All that said, I would support having Query's constructor take a single array of options instead of separate arguments, as I know having a constructors with many args (many of which are optional in their own right) is asking for trouble down the line if we need to change the API.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$limit, $skip order will forever require quick documentation lookup.
I foresee $queryFields becoming part of a $queryDocument in the near future, on the same level as $orderyBy

I think having these as separate arguments will is indeed asking for problems down the line.
I would prefer an array of "our own" query options so we do not tie us into a dedicated implementation detail on how the current query and opcode wire protocol works.

I don't understand the difference between your suggested alternative vs the original suggestion.. So refactor this into an array of our own query options as you suggested please :)


$readPreference = array(
"type" => 'secondaryPreferred',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read preferences are strings, so I don't think the base driver needs to enforce constants. That is easily maintained in PHP userland, unless we need to throw exceptions for unrecognized read preferences?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think it was a bad call to make it an array.
It should be ReadPreference class. Validating this array will be a source of a mess otherwise.
Typehinting on ReadPreferences solves that completely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To that end, shouldn't WriteOptions and possible WriteConcern also have their own class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes :)

"tags" => array(
array("dc:eu", "cpu:fast"),
array("dc:eu", "cc:is"),
array(""),
),
);

$mm = new \MongoDB\Manager("mongodb://server1,server2/?replicaSet=FOOBAR");

$cursor = $mm->executeQuery("db.collection", $query, $readPreference);

assert($cursor instanceof \MongoDB\Cursor);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, this was QueryResult. I'm not sure what the relation of that class would be to the result of a command, which would also be iterable (i.e. Traversable in PHP) and possibly a cursor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking an intermediate step from query execution to the actual cursor.
I can imagine if a query becomes a command in the future the return value of that would be a cursor document, along with maybe some statistics such as execution time and timestamp of the last entry in the oplog so a user can decide to discard the results and try another secondary instead or whatever the future may hold.

QueryResult would implement IteratorAggregate and return a Cursor which would only be that, a iterable cursor.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense and I believe we talked about that, too. It wasn't clear from the foreach that we'd rely on IteratorAggregate :)


foreach($cursor as $person) {
printf(
"%s has lived in %s\n",
$person->getName(),
$person->getAddresses()[0]->getCountry()->getName()
);
}