-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GEOS-8069] Add documentation and tests about using MongoDB as a data store for app-schema #2205
Conversation
@@ -0,0 +1,273 @@ | |||
package org.geoserver.test.onlineTest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmco I do not know much about MongoDB but this looks like a tidy and thorough piece of work.
@@ -0,0 +1,436 @@ | |||
.. _app-schema.tutorial: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate label (same as the app-schema tutorial). Should be:
.. _app-schema.mongo-tutorial:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
</DataStore> | ||
</sourceDataStores> | ||
|
||
Check the MongoDB tutorial for a more detailed description about how to use MongoDB with app-schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please link to the mongo tutorial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct thanks, fixed.
|
||
Check the MongoDB tutorial for a more detailed description about how to use MongoDB with app-schema. | ||
|
||
.. note:: You must install the Oracle plugin to connect to Oracle Spatial databases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your mean MongoDB here. Cut and paste error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup fixed thanks.
|
||
db.stations.insert({ | ||
"id": "1", | ||
"name": "station 1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma at end of line here. Hooray for GitHub syntax highlighting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
||
db.stations.insert({ | ||
"id": "2", | ||
"name": "station 2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing comma at end of line here. Hooray for GitHub syntax highlighting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
}) | ||
|
||
db.stations.createIndex({ | ||
geometry: "2dsphere" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub JSON syntax highlighting suggests that something is wrong with geometry
. Missing quotes perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -0,0 +1,36 @@ | |||
{ | |||
"id": "1", | |||
"name": "station 1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! Unlike the tutorial, this line has a comma at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah no sure what happen, I copy paste test cases to doc 😥.
@BeforeClass | ||
public static void setup() throws Exception { | ||
// check that we have access to a mongodb and instantiate the client | ||
String hostAsString = getProperty("mongo.host", "127.0.0.1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fixture should use settings stored in a properties file ~/.geoserver/mongodb.properties
or similar. GeoTools fixture configuration support is more reusable, but GeoServer is messier. ReferenceDataPostgisSetup
uses JDBCTestSetup
but I expect that this will be less useful for Mongo. GeoTools MongoTestSupport does the right thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, fixed.
Running MongoDB Online Tests | ||
---------------------------- | ||
|
||
MongoDB online tests expected by default that a MongoDB instance can be reached using host `127.0.0.1` and port `27017`. A different host can be provided using environment variable `mongo.host` and a different port can be provided using variable `mongo.port`. If no MongoDB instance can be reached the online tests will be skipped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted elsewhere, it would be preferable that these setting were stored in a mongodb.properties
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
With no MongoDB installed, this PR causes app-schema online tests (
Looking a little deeper, one fails in
and the other fails in
|
f9f36e5
to
addb080
Compare
Thanks for the review @bencaradocdavies ! I amend has requested and online should work has expected now. |
* tests hence they require a MongoDB instance. Host 127.0.0.1 and port 27017 will | ||
* be used by default but is possible to provide another host and port using Java | ||
* variables mongo.host and mongo.port respectively. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should mention mongodb.properties
not Java properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I forget about this Java doc. Fixed.
@nmco that looks great. Tidy work. Full builds and app-schema online tests allpass (without a mongodb instance). I plan to merge this and geotools/geotools#1546 as soon as the geoserver-master build on ares Jenkins has recovered (I think its last failure was some intermittent resource problem). There is the one little note about the test javadoc that could be fixed to mention |
addb080
to
d24d2af
Compare
Java doc amended and a mean-full message will be displayed pointing to the |
This pull request adds documentation bout how to use MongoDB with App-Schema and adds online tests (integration test).
Depends on pull request: geotools/geotools#1546
Associated issue: https://osgeo-org.atlassian.net/browse/GEOS-8069