8000
Skip to content

Add elasticsearch option when mongo is selected#6595

Merged
jdubois merged 10 commits intojhipster:masterfrom
ramzimaalej:MongoElasticsearchSupport
Dec 4, 2017
Merged

Add elasticsearch option when mongo is selected#6595
jdubois merged 10 commits intojhipster:masterfrom
ramzimaalej:MongoElasticsearchSupport

Conversation

@ramzimaalej
Copy link
Copy Markdown
Contributor
@ramzimaalej ramzimaalej commented Oct 25, 2017 •
edited
Loading

This is a first attempt to fix this issue #1810

@jdubois @deepu105 Let me know if you have any feedback on this. I also would like to add tests to this and solve this non-blocking issue:

org.reflections.ReflectionsException: could not create Vfs.Dir from url, no matching UrlType was found [file:/Users/rmaalej/Documents/dev/workspace/expensy/expensy-analytics/build/classes/scala/main] either use fromURL(final URL url, final List<UrlType> urlTypes) or use the static setDefaultURLTypes(final List<UrlType> urlTypes) or addDefaultURLTypes(UrlType urlType) with your specialized UrlType. at org.reflections.vfs.Vfs.fromURL(Vfs.java:109) at org.reflections.vfs.Vfs.fromURL(Vfs.java:91)

@ramzimaalej ramzimaalej force-pushed the MongoElasticsearchSupport branch 2 times, most recently from 91d4682 to 3590eaa Compare October 26, 2017 17:09
@pascalgrimaud
Copy link
Copy Markdown
Contributor
pascalgrimaud commented Oct 26, 2017

Congrats @ramzimaalej !
It's a feature a lot of people wanted since 2 years, but no one contribute to it.

@jdubois
Copy link
Copy Markdown
Member
jdubois commented Oct 26, 2017

Yes, awesome!

deepu105
deepu105 reviewed Oct 26, 2017
View reviewed changes
generators/entity/templates/server/src/main/java/package/domain/_Entity.java Outdated
}
} if (databaseType === 'mongodb') { _%>
@Document(collection = "<%= entityTableName %>")
@org.springframework.data.mongodb.core.mapping.Document(collection = "<%= entityTableName %>")
Copy link
Copy Markdown
Member
@deepu105 deepu105 Oct 26, 2017

Choose a reason for hiding this comment

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

To make it less ugly when using mongo without ES I suggest to create an intance variable called mongoDocumentAnnotation and use it here the value of which can be qualified path when using ES and just document when using without ES. The same gies for ES annotation and the imports

Copy link
Copy Markdown
Contributor Author
@ramzimaalej ramzimaalej Oct 26, 2017

Choose a reason for hiding this comment

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

sure, I will take care of it

@ramzimaalej ramzimaalej changed the title [Do not merge] Add elasticsearch option when mongo is selected Add elasticsearch option when mongo is selected Oct 27, 2017
deepu105
deepu105 reviewed Oct 27, 2017
View reviewed changes
generators/entity/templates/server/src/main/java/package/domain/_Entity.java
@Table(name = "<%= entityInstance %>")
<%_ } if (searchEngine === 'elasticsearch') { _%>
<%_ } if (searchEngine === 'elasticsearch' && databaseType === 'mongodb') { _%>
@org.springframework.data.elasticsearch.annotations.Document(indexName = "<%= entityInstance.toLowerCase() %>")
Copy link
Copy Markdown
Member
@deepu105 deepu105 Oct 27, 2017

Choose a reason for hiding this comment

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

While this is correct my idea was to simplify this with a variable. Dont worry ill do it while merging. Ill merge th 8000 is later

Copy link
Copy Markdown
Contributor Author
@ramzimaalej ramzimaalej Oct 29, 2017

Choose a reason for hiding this comment

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

I think this way offers better readability of what's going in the template unless you want to put the logic here, but it's up to you. I don't have a strong opinion about it.

@deepu105
Copy link
Copy Markdown
Member
deepu105 commented Oct 27, 2017

Please check the test failure for new setup

@ramzimaalej ramzimaalej force-pushed the MongoElasticsearchSupport branch 2 times, most recently from ef6e343 to b0259e5 Compare October 27, 2017 18:42
@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Oct 27, 2017 •
edited
Loading

@deepu105 I'm looking into the issue, it's related to mapstruct bean not being injected. How many people are using Mapstruct. I'm thinking of disabling Mapstruct when mongo and elasticsearch are selected, until we figure out the issue. @jdubois @deepu105 thoughts?

@ramzimaalej ramzimaalej force-pushed the MongoElasticsearchSupport branch from b0259e5 to 6d269e2 Compare October 28, 2017 08:30
@ramzimaalej ramzimaalej changed the title Add elasticsearch option when mongo is selected [Do not merge] Add elasticsearch option when mongo is selected Oct 28, 2017
@ramzimaalej ramzimaalej mentioned this pull request Oct 29, 2017
Closed
@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Oct 29, 2017 •
edited
Loading

Intellij was misleading me, the issue has nothing to do with Mapstruct. Actually, it's because of the ZonedDateTime converter.

`public static class DateToZonedDateTimeConverter implements Converter<Date, ZonedDateTime> {
public static final JSR310DateConverters.DateToZonedDateTimeConverter INSTANCE = new JSR310DateConverters.DateToZonedDateTimeConverter();

    private DateToZonedDateTimeConverter() {
    }

    public ZonedDateTime convert(Date source) {
        return source == null ? null : ZonedDateTime.ofInstant(source.toInstant(), ZoneId.systemDefault());
    }
}

public static class ZonedDateTimeToDateConverter implements Converter<ZonedDateTime, Date> {
    public static final JSR310DateConverters.ZonedDateTimeToDateConverter INSTANCE = new JSR310DateConverters.ZonedDateTimeToDateConverter();

    private ZonedDateTimeToDateConverter() {
    }

    public Date convert(ZonedDateTime source) {
        return source == null ? null : Date.from(source.toInstant());
    }
}`

while technically speaking the generated object represent the same timestamp, they are slightly different causing this line to fail

assertThat(fieldTestMapstructEntityEs).isEqualToComparingFieldByField(testFieldTestMapstructEntity);

I have changed to use string instead of data and it works like a charm

`public static class StringToZonedDateTimeConverter implements Converter<String, ZonedDateTime> {
public static final StringToZonedDateTimeConverter INSTANCE = new StringToZonedDateTimeConverter();

    private StringToZonedDateTimeConverter() {
    }

    public ZonedDateTime convert(String source) {
        return source == null ? null : ZonedDateTime.parse(source);
    }
}

public static class ZonedDateTimeToStringConverter implements Converter<ZonedDateTime, String> {
    public static final ZonedDateTimeToStringConverter INSTANCE = new ZonedDateTimeToStringConverter();

    private ZonedDateTimeToStringConverter() {
    }

    public String convert(ZonedDateTime source) {
        return source == null ? null : source.toString();
    }
}`

I raised a PR on jhipster core: jhipster/jhipster#31

@cbornet
Copy link
Copy Markdown
Member
cbornet commented Oct 30, 2017

The issue I see is that ES stores Date objects as Long so using a String will be less preformant and compact.
The issue with isEqualToComparingFieldByField() is that it uses the equals() method which checks the timezone of ZonedDateTime but as we converted to Date before storage we lose this information. We have a matcher TestUtil.sameInstant for the other tests that checks ZonedDateTime ignoring the timezone.
So I think it would be better to use isEqualToIgnoringGivenFields , ignoring the ZonedDateTime fields and use individual sameInstant matchers for the remaining ZonedDateTime fields.

@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Oct 30, 2017 •
edited
Loading

If we use long we will also lose the timezone, because when you create a ZonedDateTime from an Instant you need to specify the Timezone. That said, there is no way to guess the original timezone. I would rather use a string representation that preserves the original timezone and avoids adding complexity to the generator to find out which fields that are ZonedDateTime to be treated differently in assert statement.

@cbornet
Copy link
Copy Markdown
Member
cbornet commented Oct 30, 2017

Most of our DBs (currently all of them except cassandra) don't preserve the timezone and the view layer will remove it anyway so it's not a big deal to lose it.

@ramzimaalej ramzimaalej force-pushed the MongoElasticsearchSupport branch from 6d269e2 to a2ba799 Compare November 1, 2017 07:05
@ramzimaalej ramzimaalej changed the title [Do not merge] Add elasticsearch option when mongo is selected Add elasticsearch option when mongo is selected Nov 1, 2017
@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 1, 2017

done. let me know if there is anything else to address in this PR.

@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 7, 2017

@deepu105 @jdubois do you guys feel there is more stuff to do here?

@jdubois
Copy link
Copy Markdown
Member
jdubois commented Nov 8, 2017

I just need time to review but this looks quick to do

@jdubois jdubois requested a review from pascalgrimaud November 8, 2017 19:53
@pascalgrimaud
Copy link
Copy Markdown
Contributor
pascalgrimaud commented Nov 8, 2017 •
edited
Loading

LGTM
Just give a quick try, and it works well. Good job @ramzimaalej

  • generated a project with MongoDB + ES
  • generated some entities
  • launched app
  • played with entities + ES

So I don't know if this PR needs to be merged jhipster/jhipster#31

@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 8, 2017

@pascalgrimaud I think we can discard it for now (jhipster/jhipster#31).

@ramzimaalej ramzimaalej force-pushed the MongoElasticsearchSupport branch from 4d4457c to d75a61d Compare November 13, 2017 18:12
@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 14, 2017

@pascalgrimaud can you please restart the build.

@pascalgrimaud
Copy link
Copy Markdown
Contributor
pascalgrimaud commented Nov 14, 2017

it's green, so it's ok ?

@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 14, 2017 •
edited
Loading

Yeap. Once merged I will work on adding elasticsearch to couchbase as well.

@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 17, 2017

@deepu105 @jdubois any plans on when this will get merged?

@jdubois
Copy link
Copy Markdown
Member
jdubois commented Nov 17, 2017

@ramzimaalej not before the next release, where we modified our build system (with jhipster/jhipster-dependencies) and added Couchbase support. This is already a huge release, and we need some time to iron it out.
So that would be the release after.

deepu105
deepu105 requested changes Nov 26, 2017
View reviewed changes
Copy link
Copy Markdown
Member
@deepu105 deepu105 left a comment

Choose a reason for hiding this comment

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

This needs to be rebased as the entity templates are moved to entity-client and entity-server folders

@ramzimaalej ramzimaalej force-pushed the MongoElasticsearchSupport branch from d75a61d to e8f2b06 Compare November 27, 2017 03:36
@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 27, 2017

@deepu105 done

@pascalgrimaud
Copy link
Copy Markdown
Contributor
pascalgrimaud commented Nov 27, 2017

@ramzimaalej : just for your information, as soon as it's merged, I will probably move your Travis tests to https://github.com/hipster-labs/jhipster-travis-build because I'm not sure MongoDB + Elasticsearch will be used a lot. And I will add MongoDB + Elasticsearch + Gradle tests too.

I don't request change here as you already work a lot on this.
Thanks for your patience!

@ramzimaalej
Copy link
Copy Markdown
Contributor Author
ramzimaalej commented Nov 27, 2017

no prob @pascalgrimaud Let me know if you need help.

@jdubois
Copy link
Copy Markdown
Member
jdubois commented Dec 4, 2017

LGTM and @deepu105 your changes were corrected, so let's merge this.
Congrats @ramzimaalej on a huge new feature!

@jdubois jdubois merged commit af396cf into jhipster:master Dec 4, 2017
@ramzimaalej ramzimaalej deleted the MongoElasticsearchSupport branch December 4, 2017 18:13
@jdubois jdubois added this to the 4.12.0 milestone Dec 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepu105 deepu105 deepu105 requested changes

+1 more reviewer

@pascalgrimaud pascalgrimaud pascalgrimaud approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr: needs-review

Projects

None yet

Milestone

4.12.0

Development

Successfully merging this pull request may close these issues.

5 participants

@ramzimaalej @pascalgrimaud @jdubois @deepu105 @cbornet
0