[Geoserver-devel] Clean up Wicket URLs, part two

Hi,
I've been looking into ways to have Wicket automatically generate
nice looking mount points without having to manually register or
mount the urls. The thing did not go well, it is actually doable
but I found myself lost into some hairy code in the Wicket
WebRequestCodingStrategy class, which is not really meant for
easy subclassing, so I decided to stay away from it :slight_smile:
In any case, if someone wants to try, have a look at
WebRequestCodingStrategy and the only subclass of it,
UrlCompressingWebCodingStrategy, which to be used actually
needs also the UrlCompressingWebRequestProcessor class.

As an alternative I took David's patch and modified it
a bit so that:
- any page, not just menu pages, can be registered
   and mounted with a nice path, by rolling PageInfo,
   a MenuPaneInfo superclass that knows about mounting
- the nice path gets automatically computed, unless
   manually overridden, from the class package and name

The strategy makes it so that a page class like:
org.geoserver.web.data.layer.LayerPage
gets a path like:
data/layer/layer
and thus it's available at:
http://host:port/geoserver/web/data/layer/layer

Basically the convention strips off org.geoserver.web
and Page and turns what remains in the path (thus the
double layer/layer, due to layer.LayerPage).

For the current menu pages we don't have to move a finger,
but to get nice looking urls we should enforce some
better convention on class names, something like:
org.geoserver.web.data.layer.LayerListPage
org.geoserver.web.data.layer.NewLayerPage
org.geoserver.web.data.layer.EditLayerPage
which would turn into the following urls:
http://host:port/geoserver/web/data/layer/layerlist
http://host:port/geoserver/web/data/layer/newlayer
http://host:port/geoserver/web/data/layer/editlayer

Also, in the web wms/wfs/wcs modules the root package
is like org.geoserver.w?s.web, to get good names we
should again uniform to the convention and use
org.geoserver.web.w?s instead.

So it's a bit of work, but nothing so hard, and would
result in a better uniformity in the code as well.

If we don't want to go down the convention path
PageInfo also allows to manually specify a mount path.

Opinions?

Cheers
Andrea

--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.

(attachments)

pagesMounts.patch (3.03 KB)

I think that if the URL exposes the class hierarchy, then there's a bit more pressure to make that hierarchy well-named and sensible. Also, anything that makes page-exposing automatic is okay in my book. To this end I would support moving org.geoserver.w?s.web -> org.geoserver.web.w?s. I know I know, easy for me to say. :slight_smile: I am not a Java person, but this seems like moving in the right direction.

Thanks,
Mike Pumphrey
OpenGeo - http://opengeo.org

Andrea Aime wrote:

Hi,
I've been looking into ways to have Wicket automatically generate
nice looking mount points without having to manually register or
mount the urls. The thing did not go well, it is actually doable
but I found myself lost into some hairy code in the Wicket
WebRequestCodingStrategy class, which is not really meant for
easy subclassing, so I decided to stay away from it :slight_smile:
In any case, if someone wants to try, have a look at
WebRequestCodingStrategy and the only subclass of it,
UrlCompressingWebCodingStrategy, which to be used actually
needs also the UrlCompressingWebRequestProcessor class.

As an alternative I took David's patch and modified it
a bit so that:
- any page, not just menu pages, can be registered
  and mounted with a nice path, by rolling PageInfo,
  a MenuPaneInfo superclass that knows about mounting
- the nice path gets automatically computed, unless
  manually overridden, from the class package and name

The strategy makes it so that a page class like:
org.geoserver.web.data.layer.LayerPage
gets a path like:
data/layer/layer
and thus it's available at:
http://host:port/geoserver/web/data/layer/layer

Basically the convention strips off org.geoserver.web
and Page and turns what remains in the path (thus the
double layer/layer, due to layer.LayerPage).

For the current menu pages we don't have to move a finger,
but to get nice looking urls we should enforce some
better convention on class names, something like:
org.geoserver.web.data.layer.LayerListPage
org.geoserver.web.data.layer.NewLayerPage
org.geoserver.web.data.layer.EditLayerPage
which would turn into the following urls:
http://host:port/geoserver/web/data/layer/layerlist
http://host:port/geoserver/web/data/layer/newlayer
http://host:port/geoserver/web/data/layer/editlayer

Also, in the web wms/wfs/wcs modules the root package
is like org.geoserver.w?s.web, to get good names we
should again uniform to the convention and use
org.geoserver.web.w?s instead.

So it's a bit of work, but nothing so hard, and would
result in a better uniformity in the code as well.

If we don't want to go down the convention path
PageInfo also allows to manually specify a mount path.

Opinions?

Cheers
Andrea

------------------------------------------------------------------------

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge This is your chance to win up to $100,000 in prizes! For a limited time, vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize details at: http://p.sf.net/sfu/Challenge

------------------------------------------------------------------------

_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

Mike Pumphrey ha scritto:

I think that if the URL exposes the class hierarchy, then there's a bit more pressure to make that hierarchy well-named and sensible. Also, anything that makes page-exposing automatic is okay in my book. To this end I would support moving org.geoserver.w?s.web -> org.geoserver.web.w?s. I know I know, easy for me to say. :slight_smile: I am not a Java person, but this seems like moving in the right direction.

I won't be around next week (or else, will not be around much) so
if someone is eager to take this up, open a jira,
fix and test at will, and commit, be my guest.

Cheers
Andrea

--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.

Looked a bit into this as well, and I agree it is tricky. One thing i am not crazy about with the proposed solution is putting the logic directly into the ComponentInfo (PageInfo) class. This class is designed to be relatively dumb, just metadata about the page. I think it makes more sense to keep all the mounting logic separate so it will be easier to swap it out. Baking it directly into ComponentInfo I fear it is something we will be stuck with since it is so central.

As a middle ground i would say perhaps keep mountPath on PageInfo, but move all the logic for figuring out what a sensible default is into GeoServerApplication.

In terms of url mappings what about the slightly less verbose:

LayerListPage -> .../layer/list
LayerNewPage -> .../layer/new
LayerEditPage -> .../layer/edit

Also, for the edit case, don't we have to encode some state in the url? The id of the object being edited?

I took the opportunity to play around expanding on the patch and here is what I came up with:

1) Add PageInfo same as the patch

2) Add PageInfo instances to the app context for all pages, not just ones linked from the main menu

3) In GeoServerApplication.init() do the dance of stripping off the package name, and inspect the class name to figure if it is a "list", "edit" , or "new" case. And mount the page class appropriately.

The "edit" case is a bit tricky because it has state which needs to be represented in the url. Something like:

   http://…/layer/edit?id=<layerid> or:
   http://…/layer/edit/<layerid>

Now at the end of the day all the edit pages basically hold onto the id of the object they are editing (I think some hold onto the actual object but I think there is a jira open to fix this, and make the behavior the same). Those pages that hold onto the id usually do so with a LoadableDetachable model. There is WorkspaceDetachableModel, LayerDetachableModel, etc...

All those loadable detachable models could extend a single base class, something like "CatalogDetachableModel", which would look something like:

class CatalogDetachableModel implements LoadableDetachableModel {

   String id;

   protected CatalogDetachableModel(String id) {
     this.id = id;
   }

   String getID() {
     return id;
   }

   final Object load() {
      return loadFromId(id);
   }

   protected abstract Object loadFromId(String id);

}

Now, in step 3 above, when GeoServerApplication is mounting all the classes from PageInfo, and it detects the edit case it would mount a specific implementation of IRequestTargetUrlCodingStrategy which in its encode() method calls page.getModel(), checks it for being an instanceof CatalogDetachableModel, and gets the id from it.

The decode() method does the opposite, takes the id from the request and created the page from it.

Thoughts? The additional work will be to make all the LoadableDetachableModels for catalog and config objects uniform, along with the edit pages that use them, but i think that is a plus.

-Justin

Andrea Aime wrote:

Hi,
I've been looking into ways to have Wicket automatically generate
nice looking mount points without having to manually register or
mount the urls. The thing did not go well, it is actually doable
but I found myself lost into some hairy code in the Wicket
WebRequestCodingStrategy class, which is not really meant for
easy subclassing, so I decided to stay away from it :slight_smile:
In any case, if someone wants to try, have a look at
WebRequestCodingStrategy and the only subclass of it,
UrlCompressingWebCodingStrategy, which to be used actually
needs also the UrlCompressingWebRequestProcessor class.

As an alternative I took David's patch and modified it
a bit so that:
- any page, not just menu pages, can be registered
  and mounted with a nice path, by rolling PageInfo,
  a MenuPaneInfo superclass that knows about mounting
- the nice path gets automatically computed, unless
  manually overridden, from the class package and name

The strategy makes it so that a page class like:
org.geoserver.web.data.layer.LayerPage
gets a path like:
data/layer/layer
and thus it's available at:
http://host:port/geoserver/web/data/layer/layer

Basically the convention strips off org.geoserver.web
and Page and turns what remains in the path (thus the
double layer/layer, due to layer.LayerPage).

For the current menu pages we don't have to move a finger,
but to get nice looking urls we should enforce some
better convention on class names, something like:
org.geoserver.web.data.layer.LayerListPage
org.geoserver.web.data.layer.NewLayerPage
org.geoserver.web.data.layer.EditLayerPage
which would turn into the following urls:
http://host:port/geoserver/web/data/layer/layerlist
http://host:port/geoserver/web/data/layer/newlayer
http://host:port/geoserver/web/data/layer/editlayer

Also, in the web wms/wfs/wcs modules the root package
is like org.geoserver.w?s.web, to get good names we
should again uniform to the convention and use
org.geoserver.web.w?s instead.

So it's a bit of work, but nothing so hard, and would
result in a better uniformity in the code as well.

If we don't want to go down the convention path
PageInfo also allows to manually specify a mount path.

Opinions?

Cheers
Andrea

------------------------------------------------------------------------

------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge This is your chance to win up to $100,000 in prizes! For a limited time, vendors submitting new applications to BlackBerry App World(TM) will have
the opportunity to enter the BlackBerry Developer Challenge. See full prize details at: http://p.sf.net/sfu/Challenge

------------------------------------------------------------------------

_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Justin Deoliveira ha scritto:

Looked a bit into this as well, and I agree it is tricky. One thing i am not crazy about with the proposed solution is putting the logic directly into the ComponentInfo (PageInfo) class. This class is designed to be relatively dumb, just metadata about the page.

I have no troubles with moving behavior outside the class.
Yet, having all these pure data holders around makes me wonder,
the issue of pure data holders is usually described as
"anemic domain model":
http://en.wikipedia.org/wiki/Anemic_Domain_Model
http://martinfowler.com/bliki/AnemicDomainModel.html

As a middle ground i would say perhaps keep mountPath on PageInfo, but move all the logic for figuring out what a sensible default is into GeoServerApplication.

Works for me

In terms of url mappings what about the slightly less verbose:

LayerListPage -> .../layer/list
LayerNewPage -> .../layer/new
LayerEditPage -> .../layer/edit

I guess we can, how would the general name transformation rule
look like? Would we have to check if the page name starts with the
same name as the containing package?

Also, for the edit case, don't we have to encode some state in the url? The id of the object being edited?

Yes, we would, thought I did not check how the resulting URL would look like.

I took the opportunity to play around expanding on the patch and here is what I came up with:

1) Add PageInfo same as the patch

2) Add PageInfo instances to the app context for all pages, not just ones linked from the main menu

Yep. At least for all pages that we want to make bookmarkable

3) In GeoServerApplication.init() do the dance of stripping off the package name, and inspect the class name to figure if it is a "list", "edit" , or "new" case. And mount the page class appropriately.

Roger

The "edit" case is a bit tricky because it has state which needs to be represented in the url. Something like:

  http://…/layer/edit?id=<layerid> or:
  http://…/layer/edit/<layerid>

Now at the end of the day all the edit pages basically hold onto the id of the object they are editing (I think some hold onto the actual object but I think there is a jira open to fix this, and make the behavior the same). Those pages that hold onto the id usually do so with a LoadableDetachable model. There is WorkspaceDetachableModel, LayerDetachableModel, etc...

All those loadable detachable models could extend a single base class, something like "CatalogDetachableModel", which would look something like:

class CatalogDetachableModel implements LoadableDetachableModel {

  String id;

  protected CatalogDetachableModel(String id) {
    this.id = id;
  }

  String getID() {
    return id;
  }

  final Object load() {
     return loadFromId(id);
  }

  protected abstract Object loadFromId(String id);

}

Now, in step 3 above, when GeoServerApplication is mounting all the classes from PageInfo, and it detects the edit case it would mount a specific implementation of IRequestTargetUrlCodingStrategy which in its encode() method calls page.getModel(), checks it for being an instanceof CatalogDetachableModel, and gets the id from it.

At that point the page instances are still not created, and the model not set. How would this work?

Cheers
Andrea

--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.

Andrea Aime wrote:

Justin Deoliveira ha scritto:

Looked a bit into this as well, and I agree it is tricky. One thing i am not crazy about with the proposed solution is putting the logic directly into the ComponentInfo (PageInfo) class. This class is designed to be relatively dumb, just metadata about the page.

I have no troubles with moving behavior outside the class.
Yet, having all these pure data holders around makes me wonder,
the issue of pure data holders is usually described as
"anemic domain model":
http://en.wikipedia.org/wiki/Anemic_Domain_Model
http://martinfowler.com/bliki/AnemicDomainModel.html

Hmmm... guess I am a bad programmer :slight_smile:

The reason I try to stick to this separation can be summed up by one class: org.vfny.geoserver.config.FeatureTypeInfo. Which was a mix of data/java bean and logic. Again, maybe i don't know what I am doing so take this for what its worth, but I felt that this class was a nightmare to maintain. And in porting to the new configuration and catalog, believe me when I say that ensuring that all the logic and assumptions that this class encapsulated was no simple task.

That said, I know I push this anti-pattern quite extensively in my designs and implementation, so if others feel that this does more bad than good I will gladly change my practices.

As a middle ground i would say perhaps keep mountPath on PageInfo, but move all the logic for figuring out what a sensible default is into GeoServerApplication.

Works for me

In terms of url mappings what about the slightly less verbose:

LayerListPage -> .../layer/list
LayerNewPage -> .../layer/new
LayerEditPage -> .../layer/edit

I guess we can, how would the general name transformation rule
look like? Would we have to check if the page name starts with the
same name as the containing package?

Well... here is what i see. Let's call the "prefix" in this case "layer". And the "operation" one of (list,new,edit)

To get the prefix:

1. Strip off org.geoserver.web from the package
2. Take the last component of the package remaining (ie, "data.layer" -> "layer")

And to get the operation check for the classname ending in one of ListPage,EditPage,NewPage.

Also, for the edit case, don't we have to encode some state in the url? The id of the object being edited?

Yes, we would, thought I did not check how the resulting URL would look like.

I took the opportunity to play around expanding on the patch and here is what I came up with:

1) Add PageInfo same as the patch

2) Add PageInfo instances to the app context for all pages, not just ones linked from the main menu

Yep. At least for all pages that we want to make bookmarkable

3) In GeoServerApplication.init() do the dance of stripping off the package name, and inspect the class name to figure if it is a "list", "edit" , or "new" case. And mount the page class appropriately.

Roger

The "edit" case is a bit tricky because it has state which needs to be represented in the url. Something like:

  http://…/layer/edit?id=<layerid> or:
  http://…/layer/edit/<layerid>

Now at the end of the day all the edit pages basically hold onto the id of the object they are editing (I think some hold onto the actual object but I think there is a jira open to fix this, and make the behavior the same). Those pages that hold onto the id usually do so with a LoadableDetachable model. There is WorkspaceDetachableModel, LayerDetachableModel, etc...

All those loadable detachable models could extend a single base class, something like "CatalogDetachableModel", which would look something like:

class CatalogDetachableModel implements LoadableDetachableModel {

  String id;

  protected CatalogDetachableModel(String id) {
    this.id = id;
  }

  String getID() {
    return id;
  }

  final Object load() {
     return loadFromId(id);
  }

  protected abstract Object loadFromId(String id);

}

Now, in step 3 above, when GeoServerApplication is mounting all the classes from PageInfo, and it detects the edit case it would mount a specific implementation of IRequestTargetUrlCodingStrategy which in its encode() method calls page.getModel(), checks it for being an instanceof CatalogDetachableModel, and gets the id from it.

At that point the page instances are still not created, and the model not set. How would this work?

Well, generally in the edit page class our application creates the page directly in order to set the state, rather than just supply the class of page and letting wicket create it. Correct?

When this happens the IRequestTragetUrlCodingStrategy will be asked to encode an instnace of IPageRequestTarget, which has the method getPage().

So to pull this off we would need to:

1) ensure we stick to instantiating the page before hand, although the coding strategy could pretty easily handle the case of a page class with some page parameters.

2) Ensure that in the constructor of an edit page that the model is set properly.

Just an idea... perhaps not a good one. Open to alternatives here.

Cheers
Andrea

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.