Issue when adding entities in a list

[Sydney]

I have an issue when I want to add a Review to a Product. If I call the setReview method for two different product, the review gets created with the same id. So both products have the same review whereas I need two different reviews. In my case, newReview is true, the ArrayList reviews is empty, a new Review is created and added to the reviews. The review is stored when productDao.update(product) is called. I want to know why the created review id gets an already used id. The product is partially activated.

@Override
public Boolean setReview(Product product, ProductResult result, Boolean admin) {
    OAuthUserId currentUserId = getCurrentOAuthUserId();
    boolean demoUser = currentUserId.equals(ServerConstants.DEMO_ID);
    if (!demoUser) {
        ProductDao productDao = productDaoProvider.get();
        ReviewDao reviewDao = reviewDaoProvider.get();
        List<Review> reviews = product.getReviews();
        productDao.activateAll(reviews);
        if (reviews == null) {
            reviews = new ArrayList<Review>();
            product.setReviews(reviews);
        }
        Review currentReview = null;
        for (Review review : reviews) {
            if (review.getoAuthUserId().equals(currentUserId)
                    && admin.equals(review.isAdmin())) {
                currentReview = review;
                break;
            }
        }
        boolean newReview = currentReview == null;
        if (newReview) {
            currentReview = new Review();
            currentReview.setoAuthUserId(currentUserId);
            currentReview.setAdmin(admin);
            reviews.add(currentReview);
        }
        if (result == null) {
            reviews.remove(currentReview);
            // denormalization
            productDao.activate(product.getProductParent());
            productDao.update(product);
            reviewDao.delete(currentReview);
        } else {
            currentReview.setResult(result);
            currentReview.setTimestamp(new Date());
            if (newReview) {
                // denormalization
                productDao.activate(product.getProductParent());
                productDao.update(product);
            } else {
                reviewDao.update(currentReview);
            }
        }
    }
    return !demoUser;
}

The Review entity:

public class Review extends DatastoreObject implements Serializable {
    @Index
    @Embed
    private OAuthUserId oAuthUserId;
    private ProductResult result;
    private boolean admin;
    private ProductResult verifiedResult;
    private Date timestamp;

    public Review() {
    }
}

@Entity(kind = "com.server.model.domain.DatastoreObject", allocateIdsBy = 100)
public class DatastoreObject implements Serializable {
    @Id
    protected Long id = null;
    protected Integer version = 0;

    public Long getId() {
        return id;
    }

    public void setId(Long id) {
        this.id = id;
    }

    public Integer getVersion() {
        return version;
    }

    public void setVersion(Integer version) {
        this.version = version;
    }

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((id == null) ? 0 : id.hashCode());
        return result;
    }

    @Override
    public boolean equals(Object obj) {
        if (this == obj)
            return true;
        if (obj == null)
            return false;
        if (getClass() != obj.getClass())
            return false;
        DatastoreObject other = (DatastoreObject) obj;
        if (id == null) {
            if (other.id != null)
                return false;
        } else if (!id.equals(other.id))
            return false;
        return true;
    }
}

[John Patterson]

I've skimmed over your code and can't really see an obvious problem.  You use allocateIds so I would set a breakpoint on line 555 of StandardCommonStoreCommand to see what is going on.

that line is:

datastore.encodeKeyDetails.setId(range.next().getId());

here it is setting an allocated id which should never return the same value.

[Sydney]

I have a different version of twig (beta6). In instanceToEntity, there is a call to maybeSetAllocatedId. At the end of this method datastore.encodeKeyDetails.getId() returns an available id. Then the call encoder.encode(instance, Path.EMPTY_PATH, false); changes the datastore.encodeKeyDetails.getId() value to an existing id. Then the call createEntity stores the entity with an incorrect id.

In FieldTranslator

                        PropertyTranslator translator = encoder(field, instance);
                        encoded = translator.encode(value, childPath, indexed(field));

The translator has a datastore object and encodeKeyDetails.getId() returns the available id. encoded result is an empty collections. So it's like the available id is not used.

[John Patterson]

So it sounds like the problem has already been fixed?

[Sydney]

What do you mean by fixed? in a future version?

[John Patterson]

On 28/11/2012, at 5:38 AM, Sydney wrote:

What do you mean by fixed? in a future version?

Ah OK, I thought you meant that your version (beta6) did things differently.

Looking at the code, I can see that the id is set if you have explicitly defined a non-zero id on the instance

At this line 

So probably you code is setting an id (non-zero) before you store it?

[Sydney]

I don't really understand what is going on because when encode(Object instance, Path path, boolean indexed) is called for the id of the new Review in FieldTranslator, instance is null, path is id and indexed is false. It's weird because I have this problem only for that kind of entity. For other entities when I add an instance to a list, and update the father of that list, I have no issue.

What I will do is to migrate to the latest code, and see if it fixes the issue.

Is it normal in dev mode, if two entities of different kinds have the same id? How do you make sure that an id is available for a kind?

[John Patterson]

I

On 28/11/2012, at 2:11 PM, Sydney wrote:

I don't really understand what is going on because when encode(Object instance, Path path, boolean indexed) is called for the id of the new Review in FieldTranslator, instance is null, path is id and indexed is false.

It's weird because I have this problem only for that kind of entity. For other entities when I add an instance to a list, and update the father of that list, I have no issue.

What I will do is to migrate to the latest code, and see if it fixes the issue.

That could be a good idea.  There are a lot of fixes in this last couple of weeks.

Is it normal in dev mode, if two entities of different kinds have the same id?

Id is only unique per kind and per parent.  So if you have a parent entity then the ids can be the same.

How do you make sure that an id is available for a kind?

Do you mean that every instance has a unique id regardless of its kind?  You could do it technically, when you ask the datastore for a bunch of pre-allocated ids you tell it the kind name.  This kind name can be anything (does not even need to be a kind you use).  It could be "Give me a new Id!"… get it?

You would need to implement that yourself.  But even then you could not load an instance just by id.  You would still needs its kind, and maybe parent key, to construct its key to get(Key)

[John Patterson]

On 28/11/2012, at 2:34 PM, John Patterson wrote:

I don't really understand what is going on because when encode(Object instance, Path path, boolean indexed) is called for the id of the new Review in FieldTranslator, instance is null, path is id and indexed is false.

Just a thought… do you have an @Embed ed entity that also defines an @Id?  That would over-write the existing id

[Sydney]

The Review entity has an @Embed field oAuthUserId. All my entities extend from DatastoreObject which defines a @Id field.

public class Review extends DatastoreObject implements Serializable {
    @Index
    @Embed
    private OAuthUserId oAuthUserId;
    private ProductResult result;
    private boolean admin;
    private ProductResult verifiedResult;
    private Date timestamp;
}

@Entity(kind = "com.server.model.domain.DatastoreObject", allocateIdsBy = 100)
public class DatastoreObject implements Serializable {
    @Id
    protected Long id = null;
    protected Integer version = 0;
}

public class OAuthUserId extends DatastoreObject implements Serializable {
    @Index
    private String userId;
    @Index
    private OAuthProvider provider;
}

[John Patterson]

Aha, that is it!  You have 2 @Id fields for one entity which does not make sense.

I have added a check so that this is detected and an exception thrown when any @Id is declared in an @Embed ed class.

[Sydney]

The good news is that you find out the issue, the bad news is that I have to change my data model. I would like to keep all my domain objects extending a `DatastoreObject`. The issue here is that `OAuthUserId` can be used as a embedded and as a normal entity. Any idea on how to change the model.

[Sydney]

I removed the @Id from the embedded entity, and it's working. I don't have to change my model since OAuthUserId is only used as embedded which makes sense. I was wondering what will be the impact of that change (removing the @Id from OAuthUserId) for the existing data.

[John Patterson]

Cool, I have added a sanity check so that this situation will throw an Exception from now on.

[Sydney]

I checked the current data in the datastore, and there is no entity OAuthUserId, so I guess removing the @Id will have no impact on existing data since that property was never stored. What do you think?

[John Patterson]

On 29/11/2012, at 5:41 PM, Sydney wrote:

I checked the current data in the datastore, and there is no entity OAuthUserId, so I guess removing the @Id will have no impact on existing data since that property was never stored. What do you think?

That sounds right.  What I do for for Credentials like OAuthUserId is have them implement a Credential interface and declare it @Unique so it is impossible for the same user to register twice (or two users use the same credential). @Unique is a new annotation that causes Twig to check in a transaction that the entity does not already exist.

Then in my Account class I have a List<Credential> which could e.g. contain a Facebook credential or a Google User or whatever.  This way a single account could have multiple ways of logging in to the system.