Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

UUID generation change #109

Merged
merged 7 commits into from Nov 13, 2013
Merged

UUID generation change #109

merged 7 commits into from Nov 13, 2013

Conversation

matzew
Copy link
Contributor

@matzew matzew commented Nov 12, 2013

  • Moving PD and pushApp/variantID (UUID) generation into the model/entity classes;
  • getting rid of hibernate annotations
  • cleaning up code and tests
  • added new test to test PK/ID generation

Was done for:

…etting rid of hibernate annotations; cleaning up code and tests; added new test to test PK/ID generation
@sebastienblanc
Copy link
Member

Since we move all UUID generation into the model, what about the reset https://github.com/aerogear/aerogear-unifiedpush-server/blob/UUID_Generation/src/main/java/org/jboss/aerogear/unifiedpush/rest/registry/applications/AbstractVariantEndpoint.java#L64 ?
The model could have a resetSecret() method but I don't know what's the opinion of everyone of putting this kind of logic in a model class.

@@ -44,8 +44,6 @@
/**
* Identifier used to register variants with this PushApplication
*/
void setPushApplicationID(String pushApplicationID);
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep setters? They are handy/needed for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kpiwko done

@matzew
Copy link
Contributor Author

matzew commented Nov 12, 2013

@sebastienblanc not sure; the set(Master)Secret() would be still there

public PersistentObject() {
id = UUID.randomUUID().toString();
}

Copy link
Member

Choose a reason for hiding this comment

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

You could move initialisation to the field:
private String id = UUID.randomUUID().toString();

@GenericGenerator(name = "system-uuid", strategy = "uuid")
@Column(name = "id", updatable = false, nullable = false)
private String id = null;
private String id = UUID.randomUUID().toString();
Copy link
Member

Choose a reason for hiding this comment

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

Premature optimization is evil - but could not resist here ;-)

UUID.randomUUID() is quite expensive operation, wouldn't it be possible to skip generation for entity creation when field is actually retrieved from DB? Same holds for other UUID generations.

Copy link
Member

Choose a reason for hiding this comment

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

@kpiwko the id is generated when the class is loaded, putting it in the getter will delay the generation by milliseconds / maybe less.

Copy link
Member

Choose a reason for hiding this comment

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

the point was to generate only if null - so generation would not happen if id is retrieved from database. Current impl wil generate UUID and then replace it with value stored in database. See https://issues.jboss.org/browse/AGPUSH-444

Copy link
Member

Choose a reason for hiding this comment

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

your right that is waste of cpu power we could change it, but then we would have to tell jpa to use the getter instead of the field, like this:

    private String id = null;

    @Id
    public String getId() {
        if (id == null) {
          id = UUID.randomUUID().toString();
        }
        return this.id;
    }

but then I get an error in the test MappingException: Could not determine type for: java.util.Set, at table: AbstractVariant, for columns: [org.hibernate.mapping.Column(installations)]

So let's leave that as an issue for possible improvement

@edewit edewit closed this Nov 13, 2013
@edewit edewit reopened this Nov 13, 2013
@edewit
Copy link
Member

edewit commented Nov 13, 2013

oops wrong button

@matzew matzew merged commit 97a6242 into master Nov 13, 2013
@matzew matzew deleted the UUID_Generation branch November 13, 2013 09:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants