Builder pattern for users in document database

Question

I'm attempting to make a builder pattern that makes sense and is practical for storing user data in a flexible way in a document database. The DB I chose is MongoDB, and I'm using its Java API throughout. I plan on replicating this kind of pattern once I have something that is decent.

This works, but feels a bit clunky. I would like to hear from others who have implemented builder patterns before and see what their thoughts are. Also, all other advice is welcome on all parts of the code. Here is a link to the primary class from the API that I'm using: BasicDBObjectBuilder

My tests explain fairly well how the class works and how it is used. I have also added documentation in the main class to explain as good as possible. This is Groovy, but a lot of it is actually from Java, aside from the more "sugary" syntax. I'm not using much Groovy "magic" as it is. I added a small example usage to illustrate.

Example usage:

String username = "foo"
String password = "bar"
Map details = [
        "email": "[email protected]",
        "favorite_number": 42,
        "favorite_colors": ["green", "blue"]
]

UserDocumentBuilder builder = new UserDocumentBuilder(username, password)
BasicDBObjectBuilder builderWithDetails = builder.begin()
builder.addDetails(builderWithDetails, details)
// get a DBObject which can then be inserted into MongoDB:
DBObject userDocument = builderWithDetails.get()
println userDocument.toString()

Prints this:

{ "user_name" : "foo" , "password_hash" : "37b51d194a7513e45b56f6524f2d51f2" , "date_created" : { "$date" : "2015-11-01T16:55:56.978Z"} , "details" : { "email" : "[email protected]" , "favorite_number" : 42 , "favorite_colors" : [ "green" , "blue"]}}

UserDocumentBuilderTest.groovy

import com.mongodb.BasicDBObjectBuilder
import com.mongodb.DBObject
import org.junit.Test
import org.junit.Before
import java.security.MessageDigest

/**
 * These tests are focused on the UserDocumentBuilder class.
 */
class UserDocumentBuilderTest {

    UserDocumentBuilder testUserDocumentBuilder
    DBObject emptyDBObject
    final String USERNAME = "myName"
    final String PASSWORD = "myPassword"
    final String HASHING_ALGORITHM = "MD5"
    final Map USER_DETAILS = [
            "hello": 1,
            "world": null,
            null: [ "foo", "bar" ],
            "emptyList": []
    ]

    @Before
    public void initialize() {
        testUserDocumentBuilder = new UserDocumentBuilder(USERNAME, PASSWORD)
        emptyDBObject = new BasicDBObjectBuilder().get()
    }

    @Test
    void testUserDocumentDataIsCorrect() {
        assert testUserDocumentBuilder.getUserName() == USERNAME
        assert testUserDocumentBuilder.getDateCreated() instanceof Date
    }
    @Test
    void testHashingAlgorithm() {
        assert testUserDocumentBuilder.getPasswordHash() == MessageDigest
                .getInstance(HASHING_ALGORITHM)
                .digest(PASSWORD.bytes)
                .encodeHex()
                .toString()
    }
    @Test
    void testBeginUserDocumentAsBasicDBObjectBuilder() {
        BasicDBObjectBuilder testBasicDBObjectBuilder
        testBasicDBObjectBuilder = testUserDocumentBuilder.begin()
        assert testBasicDBObjectBuilder instanceof BasicDBObjectBuilder
        assert testBasicDBObjectBuilder.get() != emptyDBObject
    }
    @Test
    void testAddUserDetailsToBuilder() {
        BasicDBObjectBuilder testUserDocumentBuilderWithDetails = testUserDocumentBuilder
                .begin()
        testUserDocumentBuilder
                .addDetails(testUserDocumentBuilderWithDetails, USER_DETAILS)
        assert testUserDocumentBuilderWithDetails instanceof BasicDBObjectBuilder
        assert testUserDocumentBuilderWithDetails.get() != emptyDBObject
    }
    @Test
    void testGetDBObjectFromBuilder() {
        DBObject testUserDocumentDBObject = testUserDocumentBuilder
                .begin()
                .get()
        assert testUserDocumentDBObject instanceof DBObject
        assert testUserDocumentDBObject != emptyDBObject
    }
}

UserDocumentBuilder.groovy

import com.mongodb.BasicDBObjectBuilder
import groovy.transform.ToString
import java.security.MessageDigest

/**
 * The UserDocumentBuilder is a builder pattern to facilitate the creation of user documents for insertion into
 * a document database.
 * <p>
 * At any point after calling `begin()` on the builder instance, we can `get()` on the BasicDBObjectBuilder
 * to obtain a DBObject ready to be inserted or otherwise used in the document database.
 */
@ToString(includeNames = true, includeFields = true)
class UserDocumentBuilder {

    String userName
    String passwordHash
    Date dateCreated
    BasicDBObjectBuilder userDocumentBuilder = new BasicDBObjectBuilder()

    /**
     * Constructor.
     * TODO: Look into better hashing algorithms to use instead of MD5.
     * @param userName  The name of the new user
     * @param passwordHash  The password after it is hashed
     * @param dateCreated  The date when the user is created, defaulting to "now" but can be passed another date
     */
    public UserDocumentBuilder(String userName, String passwordHash, Date dateCreated = new Date()) {
        this.userName = userName
        this.passwordHash = MessageDigest
                .getInstance("MD5")
                .digest(passwordHash.bytes)
                .encodeHex()
                .toString()
        this.dateCreated = dateCreated
    }
    /**
     * Start the user document builder and add in the basic information from constructor.
     * @return BasicDBObjectBuilder  the user DBObject builder
     */
    public BasicDBObjectBuilder begin() {
        def builder = userDocumentBuilder
                .start("user_name", userName)
                .add("password_hash", passwordHash)
                .add("date_created", dateCreated)
        return builder
    }
    /**
     * Create a separate BasicDBObjectBuilder for details, then add it to the userDocumentBuilder.
     * <p>
     * The idea is that this will be flexible enough to add practically any combination of key:value pairs,
     * including strings, numbers, other objects, and arrays. Supports null keys and values.
     * <p>
     * See UserDocumentBuilderTest for working examples.
     * @param userDocumentBuilder  The initial UserDocumentBuilder
     * @param details  a Key-Value map of details to add to the initial UserDocumentBuilder
     */
    public static void addDetails(BasicDBObjectBuilder userDocumentBuilder, Map details) {
        def detailsBuilder = new BasicDBObjectBuilder()
                .start(details)
        userDocumentBuilder.add("details", detailsBuilder.get())
    }
}

Answer 1

First criticism -- I feel like those unit tests do a very bad job of documenting the intent of the code.

@Test
void testUserDocumentDataIsCorrect() {
    assert testUserDocumentBuilder.getUserName() == USERNAME
    assert testUserDocumentBuilder.getDateCreated() instanceof Date
}

At the surface level, it's hard to recognize what is being tested here, because all of the behavior (such as it is) is located somewhere else. The assertions verify the implementation, so you are protected against refactoring that change the behavior, but there's nothing in this test that links the behavior to correctness.

I think a big part of the problem that you are having with this code is that you are trying to force a square peg into a round hole.

UserDocumentBuilder builder = new UserDocumentBuilder(username, password)
BasicDBObjectBuilder builderWithDetails = builder.begin()
builder.addDetails(builderWithDetails, details)
// get a DBObject which can then be inserted into MongoDB:
DBObject userDocument = builderWithDetails.get()
println userDocument.toString()

Given that addDetails is the only mutation that you support, you shouldn't be using a builder pattern at all -- you should be using a factory

UserDocumentFactory factory = new UserDocumentFactory(username, password);
DBObject userDocument = factory.create(details);

class UserDocumentFactory {
    // ...

    DBObject create(Map details) {
        return BasicDBObjectBuilder
        .start("user_name", this.username)

        // Since the factory never needs the raw password, maybe you hash it,
        // then pass the hashed version to the Factory constructor
        .add("password_hash", hash(this.password))

        // If all objects created by the factory really are supposed to share
        // the same date_created, then now() needs to return a data member,
        // rather than new Date()
        .add("date_created", now())

        .add("details", details)
        .get();
    }
}

Additional notes: it's a lot easier for the reader to understand static method calls if you invoke them through the class, rather than through an object instance. The string names used to denote important fields should probably be class constants or an enum. You'll want to make sure that Date() is actually giving you and ISO-8601 compliant representation of the date.

The interesting part of the Builder pattern is the interface of mutators that you use to collect state. If you isolate the mutable interface of your design...

interface UserDocumentBuilder {
    void addDetails(Map details);
}

You can see at once "oh, this interface is only adding complexity to something that used to be really simple". And that tells you that you are going the wrong direction.

Answer 2

1. Neither MongoDB's BasicDBObjectBuilder nor your UserDocumentBuilder is a Builder according to the pattern of the GoF.

2. Your name UserDocumentBuilder is somehow confusing. Especially when you then define:

   BasicDBObjectBuilder userDocumentBuilder = new BasicDBObjectBuilder()
   

   It represents a builder for a user, not a builder for a document of a user, doesn't it? Hence, I'd rather call it UserDBObjectBuilder.

3. AFAICS the only added value to BasicDBObjectBuilder is the addDetails(...) method to add keys/values from a Map rather than single key/value pairs only.

   Why don't you simply:

   public UserDBObjectBuilder extends BasicDBObjectBuilder
   

   and just overload add(...) with add(Map details) therein?

4. You supply a passwordHash to:

   public UserDocumentBuilder(..., String passwordHash, ...)
   

   and you apply another hash algorithm to this already-hash then? Why?

5. MongoDB's BasicDBObjectBuilder implementation, apart from the missing method descriptions, is questionable on its own:

   • What is the difference between add(...) and append(...)?
   • What does push(String key) do? Adding a key with a null value associated to it?
   • What if I invoke BasicDBObjectBuilder.start("key", "value").start()? Is the building process started over again by eliminating the values given at the first start(...)? → These start() methods should better be different constructors.