[Geoserver-devel] FormatOptionsKvpParser weird behaviour

GeoServer GeoServer Developer
1

Hi,
while trying to patch the dxf extension for GEOS-6402, I encountered a weird behaviour in FormatOptionsKvpParser.
Basically, that parser ignores the current service, version and doesn’t purge the list of available parser as KvpUtils.parse does, so it can happen that parsers defined by a different service are used to parse format_options parameters.

In my case, the WMS GetStyles layers parser was invoked to parse layers, which is a valid format option for the dxf module, but that parser transformed the original string into a List.

Tomorrow a new KvpParser could be defined in some new module and FormatOptionsKvpParser would use it and probably break.

I think we should find a way to filter parsers used by FormatOptionsKvpParser as we do for other request options.

Any suggestion?

Mauro

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

2

Hey Mauro,

So what about having FormatOptionsKvpParser delegating to the same KvpUtils.parse() method that the main kvp parsing chain does? I haven’t looked too deep but it seems like if we just parsed the format options string to a map and threw it into KvpUtils.parse() that would work?

···

On Tue, Jun 24, 2014 at 8:29 AM, Mauro Bartolomeoli <mauro.bartolomeoli@anonymised.com> wrote:

Hi,
while trying to patch the dxf extension for GEOS-6402, I encountered a weird behaviour in FormatOptionsKvpParser.
Basically, that parser ignores the current service, version and doesn’t purge the list of available parser as KvpUtils.parse does, so it can happen that parsers defined by a different service are used to parse format_options parameters.

In my case, the WMS GetStyles layers parser was invoked to parse layers, which is a valid format option for the dxf module, but that parser transformed the original string into a List.

Tomorrow a new KvpParser could be defined in some new module and FormatOptionsKvpParser would use it and probably break.

I think we should find a way to filter parsers used by FormatOptionsKvpParser as we do for other request options.

Any suggestion?

Mauro

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft

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

–

Justin Deoliveira
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive

3

Hi Justin,

···

2014-06-24 19:28 GMT+02:00 Justin Deoliveira <jdeolive@anonymised.com3839…>:

Hey Mauro,

So what about having FormatOptionsKvpParser delegating to the same KvpUtils.parse() method that the main kvp parsing chain does? I haven’t looked too deep but it seems like if we just parsed the format options string to a map and threw it into KvpUtils.parse() that would work?

The problem is that KvpUtils.parse expects info on the current service, version and so on to filter out unneeded parsers. My idea would be to change the KvpParser parse method signature to include an optional parsers list that FormatOptionsKvpParser (and possibly other parsers) can use to accomplish their job. Obviously KvpUtils.parse would pass down its internal parsers list to each called parser.

What do you think?

Thanks
Mauro Bartolomeoli

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

4

On Wed, Jun 25, 2014 at 1:21 AM, Mauro Bartolomeoli <
mauro.bartolomeoli@anonymised.com> wrote:

Hi Justin,

2014-06-24 19:28 GMT+02:00 Justin Deoliveira <jdeolive@anonymised.com>:

Hey Mauro,

So what about having FormatOptionsKvpParser delegating to the same
KvpUtils.parse() method that the main kvp parsing chain does? I haven't
looked too deep but it seems like if we just parsed the format options
string to a map and threw it into KvpUtils.parse() that would work?

The problem is that KvpUtils.parse expects info on the current service,
version and so on to filter out unneeded parsers. My idea would be to
change the KvpParser parse method signature to include an optional parsers
list that FormatOptionsKvpParser (and possibly other parsers) can use to
accomplish their job. Obviously KvpUtils.parse would pass down its internal
parsers list to each called parser.

Right, of course.

What do you think?

Makes sense, so what would this look when calling it from a service
specific parser? Something like:

  KvpUtils.parse(kvpMap, KvpUtils.parsers("wms", ...))

?

Thanks
Mauro Bartolomeoli

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

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

--
*Justin Deoliveira*
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive <https://twitter.com/j_deolive&gt;

5

Hi,

···

2014-06-25 15:28 GMT+02:00 Justin Deoliveira <jdeolive@anonymised.com>:

No, wait, I would’t change KvpUtils.parse signature, but KvpParser.parse.

Currently FormatOptionsKvpParser is building a list of parsers internally with:

GeoServerExtensions.extensions(KvpParser.class, applicationContext)

that obviously picks up all the parsers defined in spring contexts.

What I would do is:

  • change the signature of KvpParser.parse(String value) to KvpParser.parse(String value, List parsers)
  • update parse signature of all parsers extending KvpParser (they would basically ignore the new argument)
  • change FormatOptionsKvpParser parse method to use the passed down list of parsers, instead of the internally built one if the argument is not null
  • change KvpUtils.parse to call, for each used parser:

parser.parse(value, parsers)

where parsers is an already “filtered by service / version / request” list that KvpUtils is building to do its job.

What do you think?

Mauro

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

Right, of course.

Makes sense, so what would this look when calling it from a service specific parser? Something like:

KvpUtils.parse(kvpMap, KvpUtils.parsers(“wms”, …))

?

at options string to a map and threw it into KvpUtils.parse() that would work?

The problem is that KvpUtils.parse expects info on the current service, version and so on to filter out unneeded parsers. My idea would be to change the KvpParser parse method signature to include an optional parsers list that FormatOptionsKvpParser (and possibly other parsers) can use to accomplish their job. Obviously KvpUtils.parse would pass down its internal parsers list to each called parser.

What do you think?

6

On Wed, Jun 25, 2014 at 8:08 AM, Mauro Bartolomeoli <
mauro.bartolomeoli@anonymised.com> wrote:

Hi,

2014-06-25 15:28 GMT+02:00 Justin Deoliveira <jdeolive@anonymised.com>:

at options string to a map and threw it into KvpUtils.parse() that

would work?

The problem is that KvpUtils.parse expects info on the current service,
version and so on to filter out unneeded parsers. My idea would be to
change the KvpParser parse method signature to include an optional parsers
list that FormatOptionsKvpParser (and possibly other parsers) can use to
accomplish their job. Obviously KvpUtils.parse would pass down its internal
parsers list to each called parser.

Right, of course.

What do you think?

Makes sense, so what would this look when calling it from a service
specific parser? Something like:

  KvpUtils.parse(kvpMap, KvpUtils.parsers("wms", ...))

?

No, wait, I would't change KvpUtils.parse signature, but KvpParser.parse.

Currently FormatOptionsKvpParser is building a list of parsers internally
with:

GeoServerExtensions.extensions(KvpParser.class, applicationContext)

that obviously picks up all the parsers defined in spring contexts.

What I would do is:

- change the signature of KvpParser.parse(String value) to
KvpParser.parse(String value, List<KvpParser> parsers)
- update parse signature of all parsers extending KvpParser (they would
basically ignore the new argument)
- change FormatOptionsKvpParser parse method to use the passed down list
of parsers, instead of the internally built one if the argument is not null
- change KvpUtils.parse to call, for each used parser:

parser.parse(value, parsers)

where parsers is an already "filtered by service / version / request" list
that KvpUtils is building to do its job.

What do you think?

I am thinking that changing the signature of KvpParser is a pretty invasive
change, and that we should avoid it if possible. I am also thinking it is
kind of letting some implementation details creep into what is now a pretty
simple interface.

Perhaps I am not understanding a subtly in the problem but what I would
rather do is something like the following.

- Expose the functionality from KvpUtils that filters out parsers based on
service, request, version, etc... so that it can be used in other contexts,
like in this case by FormatOptionsKvpParser.

- Overload KvpUtils.parse with a version that takes a explicit/pre-filtered
set of parsers

I think one of the problems will be how does FormatOptionsKvpParser get the
current service/version/etc... info. Probably easiest I can think of is to
get from the thread local Request object, which should have all that info
set by the time FormatOptionsKvpParser executes.

Mauro

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

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

--
*Justin Deoliveira*
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive <https://twitter.com/j_deolive&gt;

7

Hi Justin,

···

Ok, it seems a better solution than mine.
I will try to move in this direction.

Thank you very much.

Mauro

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

I am thinking that changing the signature of KvpParser is a pretty invasive change, and that we should avoid it if possible. I am also thinking it is kind of letting some implementation details creep into what is now a pretty simple interface.

Perhaps I am not understanding a subtly in the problem but what I would rather do is something like the following.

  • Expose the functionality from KvpUtils that filters out parsers based on service, request, version, etc… so that it can be used in other contexts, like in this case by FormatOptionsKvpParser.

  • Overload KvpUtils.parse with a version that takes a explicit/pre-filtered set of parsers

I think one of the problems will be how does FormatOptionsKvpParser get the current service/version/etc… info. Probably easiest I can think of is to get from the thread local Request object, which should have all that info set by the time FormatOptionsKvpParser executes.

8

On Wed, Jun 25, 2014 at 8:51 AM, Mauro Bartolomeoli <
mauro.bartolomeoli@anonymised.com> wrote:

Hi Justin,

I am thinking that changing the signature of KvpParser is a pretty
invasive change, and that we should avoid it if possible. I am also
thinking it is kind of letting some implementation details creep into what
is now a pretty simple interface.

Perhaps I am not understanding a subtly in the problem but what I would
rather do is something like the following.

- Expose the functionality from KvpUtils that filters out parsers based
on service, request, version, etc... so that it can be used in other
contexts, like in this case by FormatOptionsKvpParser.

- Overload KvpUtils.parse with a version that takes a
explicit/pre-filtered set of parsers

I think one of the problems will be how does FormatOptionsKvpParser get
the current service/version/etc... info. Probably easiest I can think of is
to get from the thread local Request object, which should have all that
info set by the time FormatOptionsKvpParser executes.

Ok, it seems a better solution than mine.
I will try to move in this direction.

Thank you very much.

Cool, glad I could help. Thanks for taking the time to discuss!

Mauro

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/NWWaa2 for more information.

Dott. Mauro Bartolomeoli
@mauro_bart
Senior Software Engineer

GeoSolutions S.A.S.
Via Poggio alle Viti 1187
55054 Massarosa (LU)
Italy
phone: +39 0584 962313
fax: +39 0584 1660272

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

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

--
*Justin Deoliveira*
Vice President, Engineering | Boundless
jdeolive@anonymised.com
@j_deolive <https://twitter.com/j_deolive&gt;