search

openjump-gis / openjump

OpenJUMP, the Open Source GIS with more than one trick in its kangaroo pocket, takes the leap from svn to git. join the effort!
http://openjump.org
GNU General Public License v2.0
28 stars 14 forks source link

Querying database throws exception #60

Closed mukoki closed 1 year ago

mukoki commented 1 year ago

Recent versions throw the following exception when executing a query from menu File > Execute SQL Query The problem arised between r5136 (25 august) and r5140 (28 august) but is more probably du to https://github.com/openjump-gis/openjump/commit/d9f8938c9e8c105302024e80669c3a665ed38922

Error thrown when executing a query is org.locationtech.jts.util.AssertionFailedException: If this event is being fired because you are constructing a Layer, cat will be null because you haven't yet added the Layer to the LayerManager. While constructing a layer, you should set firingEvents to false. (Layerable = Nouvelle couche SQL) at org.locationtech.jts.util.Assert.isTrue(Assert.java:46) at com.vividsolutions.jump.workbench.model.LayerManager.fireLayerChanged(LayerManager.java:530) at com.vividsolutions.jump.workbench.model.AbstractLayerable.fireLayerChanged(AbstractLayerable.java:102) at com.vividsolutions.jump.workbench.model.Layer.setDataSourceQuery(Layer.java:541) at com.vividsolutions.jump.workbench.ui.plugin.datastore.RunDatastoreQueryPlugIn.createLayer(RunDatastoreQueryPlugIn.java:105) at com.vividsolutions.jump.workbench.ui.plugin.datastore.RunDatastoreQueryPlugIn.createLayerable(RunDatastoreQueryPlugIn.java:41) at com.vividsolutions.jump.workbench.ui.plugin.datastore.AbstractAddDatastoreLayerPlugIn.run(AbstractAddDatastoreLayerPlugIn.java:33) at com.vividsolutions.jump.workbench.ui.task.TaskMonitorManager$TaskWrapper.run(TaskMonitorManager.java:151)

edeso commented 1 year ago

The problem arised between r5136 (25 august) and r5140 (28 august) but is more probably du to d9f8938

how do you figure? d9f8938 should only effectively touch File Datasources

how about just disabling firingEvents() as the error suggests?

mukoki commented 1 year ago

Seems that the regression appeared when you added line 541 to Layer com.vividsolutions.jump.workbench.model.Layer.setDataSourceQuery(Layer.java:541)

Maybe it just made an older bug visible. Do you think the fix should be made in RunDatastoreQueryPlugIn by disabling events while setting the DataSourceQuery to the Layer ? I can do that. Just wonder if the bug will not appear in other data loaders. Also I remember that we often had this exception in the past. Do you thing that it is the good way to enable/disable events during data loading or should we process events in a way that tolerate that a Layer is not yet attached to the LayerManager ?

edeso commented 1 year ago

Seems that the regression appeared when you added line 541 to Layer com.vividsolutions.jump.workbench.model.Layer.setDataSourceQuery(Layer.java:541)

right, that will cause it :)

changing metadata warrants firing the event though

Maybe it just made an older bug visible. Do you think the fix should be made in RunDatastoreQueryPlugIn by disabling events while setting the DataSourceQuery to the Layer ? I can do that.

i recall that disabling events is truly a good idea if fiddling behind the scenes. better because cleaner is it to

Just wonder if the bug will not appear in other data loaders. Also I remember that we often had this exception in the past. Do you thing that it is the good way to enable/disable events during data loading

at least during layer creation definitely (as the assertion explains pretty well)

or should we process events in a way that tolerate that a Layer is not yet attached to the LayerManager ?

surely the assertion is a bit overzealous, but on the other hand points out problems early. Category seems to be mandatory for creating LayerEvents https://github.com/openjump-gis/openjump/blob/bd1235d7a30848d5dc5ce2f5df9a1fd253a9ad47/src/com/vividsolutions/jump/workbench/model/LayerManager.java#L506-L521 or https://github.com/openjump-gis/openjump/blob/bd1235d7a30848d5dc5ce2f5df9a1fd253a9ad47/src/com/vividsolutions/jump/workbench/model/LayerEvent.java#L49-L58

so no way around it. don't wanna test how category==null safe our Layerlisteners all over the place are.

tl;dr i could live with catching the assertion exception and error print it to log only skipping the event in that case. still a message, but code will works as it did before only with a message in the log to do it "better"

so should we do both . catch/log in LayerManager.java#L506 and . fixup RunDatastoreQueryPlugIn ?

mukoki commented 1 year ago

I deactivated layerEventListener in RunDatastoreQueryPlugIn. Finally, I'm not sure about replacing error by log in the case of null category. I think the probability it breaks later in one or the other LayerListener with a NPE because the category is missing is quite high. I'll try to do some more tests without this change.

mukoki commented 1 year ago

Ede, I think the deactivation of layerEventListener is enough for now. Are you OK to close the ticket.

edeso commented 1 year ago

all good, i'd still vote to make the "no category"-assertion non-lethal. but alas, there are more important construction sites :)