GeoServer
  1. GeoServer
  2. GEOS-3106

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

        Hide
        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
        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
        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
        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
        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
        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
        Gabriel Roldan added a comment -

        fixed at r12563

        Show
        Gabriel Roldan added a comment - fixed at r12563

          People

          • Assignee:
            Gabriel Roldan
            Reporter:
            Gabriel Roldan
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: