GeoServer

Disabling a Store does not disable it's layers, and they can still be accessed

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.0-beta1
  • Fix Version/s: 2.0-beta2
  • Component/s: Wicket UI
  • Labels:
    None
  • Number of attachments :
    1

Description

What the correct behaviour would be when disabling a Store?
Currently that doesn't imply disabling it's layers, so disabling a store and going to the layer preview page still shows up the layers from the disabled store, and they can be viewed.

Two options:

  • disabling a store disables it's layers
  • or disabling a sore does not disable it's layers but we add logic to avoid serving layers from disabled stores.

For instance, both LayerInfo and ResourceInfo have a isEnabled property, but I don't see where in the code a ResourceInfo is being set to enabled (not in org.geoserver.web.data.resource).

I think option 2 is the one I like more provided that we change the isEnabled property semantics to be:

  • ResourceInfo.isEnabled = getStore().isEnabled && this.enabled
  • LayerInfo.isEnabled = getResource().isEnabled && this.enabled

so it gets well encapsulated. What do you think?

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andrea Aime added a comment -

Yeah, option one would introduce quite some headaches workflow wise (e.g., when you re-enable the store, do you cascade that to layers? What if not all layers were enabled before the store was disabled, by re-enabling you get a situation different than before). +1 on going with option 2, works for me.

Show
Andrea Aime added a comment - Yeah, option one would introduce quite some headaches workflow wise (e.g., when you re-enable the store, do you cascade that to layers? What if not all layers were enabled before the store was disabled, by re-enabling you get a situation different than before). +1 on going with option 2, works for me.
Hide
Permalink
Gabriel Roldan added a comment -

Agreed. Your concerns on workflow are shared here. Yet, I'm not totally set to option 2 neither. Rationale is I don't quite buy the asymmetry between setEnabled and isEnabled that results in LayerInfo and ResourceInfo, and we're certainly loosing the capability of assessing what the actual state of them is. But option 1 is no no for sure.
I guess that leaves us with the only alternative of a utility method somewhere? And what if, at a given stage, a layerinfo's resource is null, or a resourceinfo's store is null? Easier to address as the semantics of a utility method and leave the Info's as dumb as possible. What do you think?

Show
Gabriel Roldan added a comment - Agreed. Your concerns on workflow are shared here. Yet, I'm not totally set to option 2 neither. Rationale is I don't quite buy the asymmetry between setEnabled and isEnabled that results in LayerInfo and ResourceInfo, and we're certainly loosing the capability of assessing what the actual state of them is. But option 1 is no no for sure. I guess that leaves us with the only alternative of a utility method somewhere? And what if, at a given stage, a layerinfo's resource is null, or a resourceinfo's store is null? Easier to address as the semantics of a utility method and leave the Info's as dumb as possible. What do you think?
Hide
Permalink
Andrea Aime added a comment -

Until the resource/publishing split is put in place I would just consider the layer and its resource a single entity. Trying to anticipate their separation before the Map are setup properly is just going to increase confusion.

Show
Andrea Aime added a comment - Until the resource/publishing split is put in place I would just consider the layer and its resource a single entity. Trying to anticipate their separation before the Map are setup properly is just going to increase confusion.
Hide
Permalink
Justin Deoliveira added a comment -

I second Gabriels comments about accessing state, and this falls into my usual argument of *Info being just dumb bean objects. That said, I Know this annoying middleground makes it hard to treat them as such. I would be ok with adding a new method:

boolean enabled()

Which does the above (option 2). But I would like us to keep isEnabled() and setEnabled() as simply accessors. The utility method works too, although less convenient to use.

Show
Justin Deoliveira added a comment - I second Gabriels comments about accessing state, and this falls into my usual argument of *Info being just dumb bean objects. That said, I Know this annoying middleground makes it hard to treat them as such. I would be ok with adding a new method: boolean enabled() Which does the above (option 2). But I would like us to keep isEnabled() and setEnabled() as simply accessors. The utility method works too, although less convenient to use.
Hide
Permalink
Gabriel Roldan added a comment -

enabled() looks like a good compromise, and it's already usual for us to indicate derived properties without get/set. I'd go for it if nobody objects

Show
Gabriel Roldan added a comment - enabled() looks like a good compromise, and it's already usual for us to indicate derived properties without get/set. I'd go for it if nobody objects
Hide
Permalink
Gabriel Roldan added a comment -

Justin/Andrea please check the attached patch

Show
Gabriel Roldan added a comment - Justin/Andrea please check the attached patch
Hide
Permalink
Gabriel Roldan added a comment -

fixed at r12563

Show
Gabriel Roldan added a comment - fixed at r12563

People

  • Assignee:
    Gabriel Roldan
    Reporter:
    Gabriel Roldan
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: