RFC: Library/MongoDB #2378

Closed
wants to merge 6 commits into
from

Projects

None yet

5 participants

@nbaztec
Contributor
nbaztec commented Apr 2, 2013

This PR introduces capability to integrate MongoDB. More than a PR this is a RFC to see if this is even viable. Feel free to comment as you please, even if you think this is a massive bloat of epic proportions :)

I haven't churned out the documentation yet, since I've yet to ascertain it's viability. 1 or 2 examples maybe found.

I had developed this for my internal use inspired by Alex's library (and its limitations).

P.S. I have purposefully left out Query Builder since I think QB is something which should be implemented above the base class. Also there's a high probability that a Mongo database will be using complex map-reduce/aggregate queries most of the time and/or porting them directly from their intuitive JavaScript counterparts, which are best done in-place.

@narfbg narfbg commented on the diff Apr 3, 2013
system/libraries/Mongo_db.php
+ */
+defined('BASEPATH') OR exit('No direct script access allowed');
+
+/**
+ * CodeIgniter MongoDB Class
+ *
+ * A library to interface with the NoSQL database MongoDB. For more information see http://www.mongodb.org
+ *
+ * @package CodeIgniter
+ * @subpackage Libraries
+ * @category Libraries
+ * @author Nisheeth Barthwal
+ * @link
+ */
+
+class Mongo_db {
@narfbg
narfbg Apr 3, 2013 Collaborator

Description is "MongoDB", but this is "Mongo_db" ? It should be consistent (and I like the first version better).

@narfbg narfbg commented on the diff Apr 3, 2013
system/libraries/Mongo_db.php
+ * A library to interface with the NoSQL database MongoDB. For more information see http://www.mongodb.org
+ *
+ * @package CodeIgniter
+ * @subpackage Libraries
+ * @category Libraries
+ * @author Nisheeth Barthwal
+ * @link
+ */
+
+class Mongo_db {
+
+ /**
+ * Name of default config file
+ * @var string
+ */
+ const CONFIG_FILE = 'mongo_db';
@narfbg
narfbg Apr 3, 2013 Collaborator

This should be hard-coded to match the class name.

@narfbg narfbg commented on an outdated diff Apr 3, 2013
system/libraries/Mongo_db.php
+ */
+
+class Mongo_db {
+
+ /**
+ * Name of default config file
+ * @var string
+ */
+ const CONFIG_FILE = 'mongo_db';
+
+ /**
+ * The last error thrown by the database
+ *
+ * @var MongoException
+ */
+ protected $_last_error = FALSE;
@narfbg
narfbg Apr 3, 2013 Collaborator

Unles the real values are supposed to be boolean, you shouldn't use FALSE as the default value - this goes for all the properties. Especially for $_insert_id - 0 is a bogus value, just leave it NULL.

@narfbg narfbg and 1 other commented on an outdated diff Apr 3, 2013
system/libraries/Mongo_db.php
+ }
+
+ return $this;
+ }
+
+ /**
+ * Attempts to connect to the database. Builds the DSN if not already built.
+ *
+ * @return Mongo_db|FALSE
+ */
+ public function connect()
+ {
+ try
+ {
+ if (empty($this->dsn))
+ $this->build_dsn();
@narfbg
narfbg Apr 3, 2013 Collaborator

ALL if usage must use curly braces. This is not your first pull request, please read the styleguide.

@nbaztec
nbaztec Apr 3, 2013 Contributor

:embarrassed: Yes I recall, but I usually miss the occasional code-style, since it is not how I usually write that code. I just left the code-styles I missed as a later activity, after this is approved. Hope you don't mind that. I'd take care of this in-depth if you deem it "fit" and "desirable" via a separate commit.

@narfbg narfbg commented on the diff Apr 3, 2013
user_guide_src/source/libraries/mongo_db.rst
+The following is a list of all the preferences that can be set.
+
+============================= ====================== ============================ ================================================================
+Preference Default Value Options Description
+============================= ====================== ============================ ================================================================
+**dsn** No Default None The DSN connect string (an all-in-one configuration sequence).
+ Leave this blank to auto-generate the string.
+**hostname** localhost None The hostname of your database server. Often this is 'localhost'.
+**port** 27017 None The port to connect to the database.
+**username** No Default None The username used to connect to the database.
+**password** No Default None The password used to connect to the database.
+**database** No Default None The name of the database you want to connect to.
+**host_db_flag** No Default None Whether to append database flag while building the DSN.
+**db_debug** TRUE TRUE or FALSE (boolean) Whether database errors should be displayed.
+**result_set** array array or object The return format of the result set.
+**options['write_concern']** FALSE TRUE or FALSE (boolean) Extent of durable writes.
@narfbg
narfbg Apr 3, 2013 Collaborator

options[] here, but not for those above?

@nbaztec
nbaztec Apr 3, 2013 Contributor

I added the separate field options since these affect the queries while the rest affect the db connection. The options are debatable and open to changes.

@narfbg narfbg commented on the diff Apr 3, 2013
user_guide_src/source/libraries/mongo_db.rst
+
+..note:: ``find()`` only builds the cursor object, the records will be
+ returned by calling the ``result()`` method or its variants
+ (``$this->mongo_db->find('mycollection')->result()``)
+
+Once the cursor object is built, next set of operations can be applied easily
+to obtain the records::
+
+ $this->mongo_db
+ ->find('myusers, array('active' => TRUE), array('age' => 1, 'name' => 1, '_id' => -1))
+ ->sort(array('date_of_birth' => -1))
+ ->skip(5)
+ ->limit(20)
+ ->result();
+
+find_and_modify()
@narfbg
narfbg Apr 3, 2013 Collaborator

$this->mongo_db above, but not here? (I prefer this format though)

@nbaztec
nbaztec Apr 3, 2013 Contributor

I wasn't planning on documentation but at the last moment realized that I should atleast give some minimal (even if it's 10%) of insights, so yes my not-so-well documentation needs a lot of work and consistency fixes, and as mentioned I'm keeping it for the last minute. Unless you hold the opinion that people can better assess the viability looking at the documentation rather than the class itself, I'd do it last to save effort.

@narfbg
narfbg Apr 3, 2013 Collaborator

Regardless of wether you should do it now or later - your choice, no library gets merged without documentation and it needs to be good. I just have to make those suggestions as I see them.

@nbaztec
nbaztec Apr 3, 2013 Contributor

I understand, I'm just delaying it until this is deemed viable or "good enough". Else I'd do the documentation over the weekend.

@cryode
Contributor
cryode commented Apr 4, 2013

👎 Looks like a decent library, but I don't think it should be part of the core. There's already discussion about removing the shopping cart library, which is useful to way more people than a MongoDB lib. Plus the PECL extension requirements. MongoDB is full of growth and change -- this would need to be maintained more than anything else in the core.

@TheDigitalOrchard

Should be a database driver, not a library. (right?)

@cryode
Contributor
cryode commented Apr 8, 2013

Not really. Mongo isn't a relational database, so most of the query builder functionality would be pointless. It COULD be, but you'd lose out on some of Mongo's features that make it a useful tool (because you wouldn't have the proper methods for generating your queries, since you'd be limited to query builder's methods).

@narfbg
Collaborator
narfbg commented Apr 8, 2013

I specifically requested this to be implemented as a library. It just doesn't fit at all in our database classes.

@nbaztec
Contributor
nbaztec commented Apr 8, 2013

Exactly, couldn't have put it better than @cyrode. Also, taking a neutral stand, I also agree with the lack of user base and hence the motivation to make it part of the core. Might argue on the need of maintenance part, but I get what you are trying to convey here.

@narfbg
Collaborator
narfbg commented Apr 9, 2013
@nbaztec
Contributor
nbaztec commented Apr 9, 2013

Looks like a helpful link. Seems people are contended with a reference to the database connection. The only thing I don't prefer about this approach is the mixing of camelCase with the otherwise consistent convention.

@Neeke Neeke and 1 other commented on an outdated diff Apr 18, 2013
system/libraries/Mongo_db.php
+ return (is_object($this->_last_error))
+ ? $this->_last_error->getMessage()
+ : "";
+ }
+
+ /**
+ * Returns the last error code
+ *
+ * @return int
+ */
+ public function last_error_code()
+ {
+ return (is_object($this->_last_error))
+ ? $this->_last_error->getCode()
+ : 0;
+ }
@Neeke
Neeke Apr 18, 2013

This "0", why not use constants ?

@nbaztec
nbaztec Apr 18, 2013 Contributor

The method simply passes down the error code returned by the MongoDB, but this is in fact an error on my part, since Mongo uses 0 as an error code too. This should be returning NULL instead.

@Neeke Neeke commented on the diff Apr 18, 2013
system/libraries/Mongo_db.php
+ try
+ {
+ $this->db = $this->conn_id->{$this->database};
+ return TRUE;
+ }
+ catch (Exception $e)
+ {
+ $this->_last_error = $e;
+ if ($this->db_debug)
+ {
+ $this->display_error($e->getMessage());
+ }
+ }
+ }
+ return FALSE;
+ }
@Neeke
Neeke Apr 18, 2013

always,we shold return as early as possible and i will write this function like this:

public function use_db($database)
{
if (empty($database))
{
return FALSE;
}

The correct logic.
}

@narfbg narfbg closed this Aug 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment