Many thanks for this Erik. I've raised a ticket on incode-platform to import your great work, [1] ... just need to find the time to actually do it :-)
thx again Dan [1] https://github.com/incodehq/incode-platform/issues/58 On Thu, 12 Oct 2017 at 10:44 Erik de Hair <[email protected]> wrote: > Hi Dan, > > I've finally fixed it. Sorry it took so long. See my reply below. > > > On 07/16/2017 06:25 PM, Dan Haywood wrote: > > Hi Erik, > > > > Thanks for taking the time to pull out this ElasticSearch functionality > > into a new module. > > > > I don't think it's necessary to try to enumerate all possible > functionality > > ... it's far better to just let the use cases drive out the > functionality. > > So I'd rather that the currently working functionality is documented and > > made available, than try to second guess what new features might be > > required. > > > > > > Meantime, I've had a look over the code, and have some observations, some > > trivial and some less so. > > > > > > First, some trivial points: > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/ElasticSearchService.java#L58 > > > > > > - don't think there's any reason for this field to be static ? > Fixed. No, no good reason. I was playing around because at some point I > noticed multiple clients were created in our application and I wanted to > find out how this happened. > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/ElasticSearchService.java#L42 > > > > > > - "cluster.name" ... tend to put a prefix of some sort, eg > > "isis.services.audit.objects=all" suggest "isis.services.elasticsearch." > > as the prefix > Fixed > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/resources/org/isisaddons/module/elasticsearch/search/result/SearchResult.layout.xml > > > > > > for some reason in src/main/resources, whereas other layouts in > > src/main/java > > Think that src/main/java is the way to go > Fixed > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexServiceTest.java > > > > > > this is a test class, should be in src/test/java > Fixed. No test-directory existed and I didn't notice the files were > created in the java-directory. > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/AbstractIndex.java > > > > > > as a naming convention, I tend to use "Abstract" as a suffix > Fixed. > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L90 > > > > > > settings seem temporarily to be disabled/commented out? > Some setting might be application specific because different > applications might require diffent search/index behaviour. At some time > I was testing different indexing strategies to improve our search > engine. I didn't know what to do with this so I just left it there. This > could be solved by adding a hook like you mention below. > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L31 > > > > > > I tend to mark these as @Programmatic, to keep the size of the metamodel > > down (avoid > Fixed > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L123 > > > > and > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L133 > > > > and > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L157 > > > > and > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L168 > > > > > > recommend to just throw a new RuntimeException(e) so we can fail fast > > rather than swallow exceptions > Fixed. I should take a better look at exception handling. It's easy to > make a mess of that. > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexerFactory.java > > > > > > this looks like an SPI, so typically put these in an 'spi' subpackage > Fixed > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResult.java > > > > and > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResultsPage.java > > > > > > recommend making these JAXB view model rather than using @ViewModel; more > > flexible. > Fixed. Didn't use any JAXB view models yet. > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResultsPage.java#L60 > > > > > > can use @Nullable here instead of @Parameter(optionality = > > Optionality.OPTIONAL) > Fixed. Will the optionality-property disappear in future versions of > Apache Isis. Is there any difference? > > > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResult.java#L38 > > > > > > suggest use the ?: ternary operator > Fixed. > > > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java > > > > > > ? quite a few of the methods in IndexService aren't called, and/or seem > > incomplete: > > - addMappings > > - initialiseSearchEngine > > - addToBulkRequest > > > Like mentioned above 'addMappings' could be a hook to improve/change > index/search behaviour. > > We call 'initialiseSearchEngine' every night to recreate the index. Our > database is used by multiple applications (and sometimes even updated by > hand :-/ ) and these applications also update the same ElasticSearch > index. We recreate the index just to be sure it's up to date at least at > the beginning of the day. I can imagine this is not really needed for > every application. > > 'addToBulkRequest' is used by above initialisation. > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/fixture/src/main/java/org/isisaddons/module/elasticsearch/fixture/dom/ElasticSearchDemoObject.java#L67 > > > > > > the implementation could be simplified to: > > > > Bookmark bookmark = bookmarkService2.bookmarkFor(this); > > //return bookmark.getObjectType() +":"+ bookmark.getIdentifier() > > return bookmark.toString(); > > > > and then perhaps this could be moved into the caller, with Indexabe > > interface defining getIndexIsd() as optional (return null): > > > > - make > > default String getIndexId() { return null; } > > > > ie, and if returns null, then have the caller work out the Id using the > > bookmark > Fixed > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > So much for the trivial stuff; I also have some "deeper" questions on the > > design. > > > > As I understand the code, we use Indexable as an interface to determine - > > via the IndexableSubscriber - which domain objects get added to the > > ElasticSearch index. To do that, it obtains a corresponding Indexer for > > the domain object, which then returns an AbstractIndex - basically a DTO > > which is an extract of the relevant fields. This is serialized to JSON. > > > > I see that Index defines a getTenancy() property, and initially wondered > > whether that made sense, but on second thoughts I think it *does *make > > sense to include it in the JSON .. applications that don't need app > tenancy > > can just use "/" as a global value. I'd like to pull this concept of > > tenancy into a separate module within incode-platform, but that can wait > > til a later date for now. > The search search now only returns the first (I believe) 25 results from > ElasticSearch. If the current user isn't allowed to see (some of) these > results then no (or less) results will be displayed while the index > contains a lot of items matching the search criteria the user ís allowed > to see. So we have to check the tenancy while searching, so > ElasticSearch only returns items the current user is allowed to see. > > > > I also see that AbstractIndex has a getType() property. Is this > > necessary? Why not just use the ObjectSpecId (as per > > @DomainObject#objectType) of the object being indexed? > This might give problems when using multiple applications talking to the > same index. At least there must be a common way to mark the indexed > items with an id both applications understand. > > > > (One possible answer is if we wanted to bundle indexes of various > different > > domain objects - perhaps different subclasses into a single index, like a > > "roll-up". But does ElasticSearch do a query across all indexes anyway? > > If so, then probably no need to have getType() property) > > > > Still on AbstractIndex, I wonder if createJson needs to be defined upon > > it? Why not just have its caller (IndexService) do this serialization? > Probably better than let the index transform itself (this is only > something that happens in movies) > > > > > > Looking at Indexer : > > > > > https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/Indexer.java > > > > first question is, why is this an abstract class rather than an > interface ? > Right now the index-field of the Indexer-class is used by subclasses. It > could be changed to an interface but then the creation of the JSON must > be moved outside the Indexer-classes and that's the only thing they do now. > > > > > > But a deeper question is... was wondering if we can get rid of Indexer, > or > > at least remove it from the SPI? All it seems to do is to copy out > > selected properties from the domain object into the corresponding Index > > (with that Index then just being serialized out to JSON). Perhaps a > > different design would be extend @Property attribute to indicate which > > fields are to be included within the index? Then we can obtain the data > to > > be indexed via the metamodel? > > > > - or, can do this with a custom annotation (cf summernote and pdfjs), eg > > @ElasticSearchProperty > > - with additional facets > I was wondering this too but I'm not yet confident enough with the meta > model to refactor this using annotations. > > The way I implemented it now, we have to add some new classes and > register these to the IndexerFactory and add methods to the domain > objects we want to be indexed also. This is not an ideal situation. The > same for the preferred types and their boosting factor. > > > > If it is to be kept as an SPI, then perhaps allow multiple > implementations > > of IndexerFactory to be implemented (with these injected using @Inject > > List<IndexerFactory>). > > > > I have a feeling that maybe some SPI is required for settings too; maybe > > these concepts can be unified? > I don't exactly understand your question. > > > > > > ~~~~~~~~~ > > > > Hope the above is of use. > > > > As you might recall from the mini-conference, I've been pulling together > > all the existing addons into a single "platform" [1]. This is till work > in > > progress, but once I have that assembled and restructured, I intend to > put > > some coding standards/guidelines as to what should be in a framework > module > > - as you rightly point out, the code is written for re-use so can make > > fewer assumptions about how it is called, and should provide more hooks. > > > > For example, one of the things I've tried to do for any modules that > define > > entities - eg security - is to ensure that every > property/colleciton/action > > is annotated with a domain event; this makes it easy for consuming > > applications to selectively hide or disable functionality. There's also > a > > hierarchy for these domain event classes. Another example might be to > use > > title events rather than title() - I haven't done this consistently but > > would like to; again it provides a consistent hook for consuming apps. > > > > Another point is to ensure that any public API (as used by domain > objects) > > should be in a separate maven module. This avoid polluting the classpath > > of the domain modules with unnecessary implementation classes (eg the > > ElasticSearch client). > > > > What I would like to do though, if you're happy, is to bring in this > > ElasticSearch into the incode-platform. How does that sound? > Would be honored ;-) > > > > Thanks again > > > > Dan > > > > > > [1] https://github.com/incodehq/incode-platform > > > > > > On Thu, 13 Jul 2017 at 18:39 Erik de Hair <[email protected]> wrote: > > > >> Right now the module can do indexing and a basic query can be executed > >> to find some different types of demo objects and boost a particular type > >> of demo object, so it will show up at the top of the result list. > >> > >> Still have to simplify some things and write a readme/documentation. The > >> demo application is a based on the module template so it can be run as > >> every other module demo application. The only thing is: you need an > >> installed ElasticSearch cluster/node. > >> > >> The question about this elastic search module is: what features should > >> it offer? > >> > >> What i've implemented is: > >> * Manage connection to a ES cluster > >> * Indexing of entities marked as Indexable > >> * A basic search service that offers an action to find some entity based > >> on the content as defined by the 'Indexer' of that entity. > >> * A search results page to show the results in descending order of the > >> search score and to refine your previous search. > >> * Boosting of some preferred type to give it a higher score. > >> > >> Writing a framework module is something different than implementing > >> one's company's domain model. It's hard to decide how things should be > >> done and to determine the level of how generic it must be. So far I've > >> only been trying to let it do what it should do. Didn't spend much time > >> on the architecture. Reviews/comments very welcome! > >> > >> Erik > >> > >> > >> On 06/22/2017 03:13 PM, Erik de Hair wrote: > >>> Hi all, > >>> > >>> As promised, I started working on an ElasticSearch module. It can be > >>> found at [1]. There's still a lot to do but at least it's running and > >>> indexing demo objects. I'll keep you updated about the progress. I > >>> probably have to write a README before you can give it a try. > >>> > >>> Erik > >>> > >>> [1] https://github.com/erikdehair/isis-module-elasticsearch > >> > >
