Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supported request encodings are made configurable for WFS #544

Merged
merged 23 commits into from
Oct 1, 2015

Conversation

dstenger
Copy link
Contributor

This new features enables following functionality:
Support of request encodings (such as XML, KVP and SOAP) can be disabled in WFS service configuration (default status is that all encodings are supported). Each encoding can be disabled for every single request type (e.g. GetCapabilities) separately.
If an encoding is disabled the DCP URL won't appear in the WFS capabilities document anymore and the service throws an OperationNotSupported exception if a user tries to use that encoding.
It is not checked if the configuration is valid against the WFS specification! So, the user has do this check by himself when the new feature is used.

@MrSnyder
Copy link
Contributor

Very nice. Looks like a very reasonable addition to me.

My 2 cents:

  • Personally (as a user), I may even want to have an option that allows to disable offered/supported WFS requests (and encodings) rather than just request encodings (so I can disable certain requests altogether).
  • I consider this option to be part of the WFS protocol aspect, so I would expect it just below SupportedVersions. Maybe it could be named "SupportedRequests". If omitted, all requests (and encodings) would be enabled (just like it happens with the versions).
  • The allowed requests names could be constrained using an XSD simpleType (so I cannot misspell GetCapabilities without noting it).

@tfr42 tfr42 added this to the 3.4 milestone Apr 30, 2015
@tfr42 tfr42 added needs discussion requires discussion with contributor and removed in progress labels Apr 30, 2015
@copierrj
Copy link
Member

The TMC discussed this proposed configuration element and would like to raise a few points:

  • In general we propose not to use negations in deegree configuration files. The suggested 'active' attribute of Encoding is in violation of this convention. The general rule is that when something that is optional is omitted from the configuration, then deegree enables 'everything'. If something is configured, then only that what is explicitly mentioned in the configuration is activated.
  • An encoding is a method of performing a specific request. However these requests themselves are currently not configurable. Instead of referring to requests by name we would like the requests to be included in the configuration as separate elements. This enables us to configure (=enable) specific requests and configure more specific behavior for these requests (like encodings) as attributes of these elements.

We would like to suggest the following configuration format:

<SupportedRequests encodings="kvp">
  <GetCapabilities encodings="xml,soap"/ >
  <GetFeature encodings="xml"/ >
  <DescribeFeatureType/ >
</SupportedRequest>

This configuration snipped (only) enables GetCapabilities, GetFeature and DescribeFeatureType. KVP encoding is enabled for all configured requests (by means of the encodings attribute of SupportedRequests), GetCapabilities and GetFeature are configured to support additional encodings.

Another example:

<SupportedRequests encodings="kvp,xml"/>

This enables all requests with encodings KVP and XML.

Omitting SupportedRequests would enable all request with all encodings.

cc: @stephanr

@ghost
Copy link

ghost commented May 5, 2015

Dear TMC members,

I disagree with your main argument against the pull request.

In general we propose not to use negations in deegree configuration files.

I was not able to find any information about such a convention... I would like to have that as publicly available information.

Personally I think, if the community makes a senseful pull request, which is working fine it is good to get feedback from the TMC for enhancements. But in thinking as a community suggesting completely different approach for the configuration and rejecting the pull request for that reason is not really inviting and more frustrating than fruitful for the project...
Following such a line will lead in independent forks of the deegree project as we (and others) do not have time and money to implement everything the way the TMC wants (And I know very well that in the past this was never an issue for others who contributed...)

@MrSnyder
Copy link
Contributor

Hi Sebastian,

IMHO the "negation argument" is not the strong one. Personally, I think the other aspect is more relevant in order to keep a configuration that is able to evolve in the future without breaking backwards compatibility.

I am sorry if the rules for additions are not clear enough. I totally agree and this should definitely be improved.

@MrSnyder
Copy link
Contributor

BTW: I believe the TMC would appreciate help in improving our structures, procedures and conventions. If you are interested in helping out and joining the TMC, you have my support. I believe, this would be very helpful, especially considering that you are one of the few people who actually know most parts of the configuration.

@ghost
Copy link

ghost commented May 20, 2015

I still don't see any issues with backwards compatibility as this is a completely new configuration thing...

So for this pull request the suggested solution from TMC is to reject it and completely rewrite it? Seriously!? Very frustrating for active contributors...

For the TMC support thing: Sure I can give input, but I don´t see the need to enlarge the TMC membership

@MrSnyder
Copy link
Contributor

I would expect that the configuration change does not imply that the full pull request needs to be rewritten. What needs to be changed is the XSD and the code that extracts the configuration options from the JAXB beans.

With backward compatibility, I meant "future backward compatibility". You see, this configuration aspect (offered requests/encodings) is a very basic part of every OGC service. If we start adding options that allow a sub-aspect (encodings) to be configured, it may make sense to think a little further and instead do it a little more generic. This way, we should be able to create a solution that is not so likely to be redone in the future.

After all, our configuration is much like an API. We shouldn't just add functionality. We should make sure that is evolves well and avoid conceptual changes. Otherwise, deegree configuration will become more and more bloated with lots of obsolete options. We didn't do a perfect job here in the past (e.g. WMS layer/theme configuration) and will not be able to do a perfect job in the future. But in cases, when a concept is clear-cut and there is a good understanding, we should always try to capture it in a way that allows future extensibility without obsoleting options.

And in this specific case, the TMC members agreed, that there is a relatively clear-cut concept that allows for that. I am sorry that this introduces inconveniences for you.

I suggested the TMC membership, because it may reduce this kind of friction if we had a person from lat/lon in the TMC that works actively with the deegree configuration or is involved in the actual coding of deegree extensions. Alternatively, the respective developer could join the TMC Skype call to become more involved.

@tfr42
Copy link
Member

tfr42 commented Jun 3, 2015

@dstenger @lgoltz Can you please give a short feedback too? An indication of the additional effort in the scale of low, average, high to make that PR fit to the proposed configuration would help to decide how to proceed.

@lgoltz
Copy link
Contributor

lgoltz commented Jun 9, 2015

As MrSnyder already said the effort to change the configuration as explained by copierrj affects only the XSD and parsing of the configuration. My estimation is low additional effort for the implementation.

@dstenger
Copy link
Contributor Author

We agree that the configuration concept of this new feature can be adapted to the general configuration concept of deegree.

But the proposed configuration

<SupportedRequests encodings="kvp">
  <GetCapabilities encodings="xml,soap"/ >
  <GetFeature encodings="xml"/ >
  <DescribeFeatureType/ >
</SupportedRequest>

has a major drawback: XML does not support attributes with multiple values (at least multiple values cannot be detected by schema validation).

So, I would propose following configuration for this feature (same example as above):

  <SupportedRequests>
    <SupportedEncodings>kvp</SupportedEncodings>
    <GetCapabilities>
      <SupportedEncodings>xml soap</SupportedEncodings>
    </GetCapabilities>
    <GetFeature>
      <SupportedEncodings>xml</SupportedEncodings>
    </GetFeature>
    <DescribeFeatureType/>
  </SupportedRequests>

What do you think about it? Unfortunately, this configuration looks more unhandy, but it corresponds to XML mechanisms. Do you have any other suggestions?

Another suggestion which concentrates more on the encodings itself is (as configuration above seems to be too complex):

  <SupportedEncodings>
    <GetCapabilities>xml soap</GetCapabilities>
    <GetFeature>xml</GetFeature>
    <DescribeFeatureType/>
  </SupportedEncodings>

This concept allows to limit encodings of single operations, but it is not possible to disable a bunch of request types (e.g. DescribeFeatureType is disabled as no encoding is allowed, but Transaction is still allowed with all encodings).
In addition, the last example corresponds more to the current implementation than your proposal.

@copierrj
Copy link
Member

Too bad multiple values in attributes are not possible in xml schema. Your suggested alternative (the one with the 'SupportedEncodings' subtags) is fine with me.

The second suggestion has a serious drawback: it makes it harder to introduce additional configuration parameters for requests in the future.

@dstenger
Copy link
Contributor Author

Correct, the logic of the first suggested alternative is the same as in your suggestion (just converted attributes to elements).

The implementation of this feature concentrates on disabling encodings. So, if you want to disable complete request types by this configuration, the current implementation would just disable all encodings for a specific request type. I am not sure if this is a consistent mechanism.
So, we could ignore the functionality of disabling request types and just concentrate on disabling encodings (but still keep the first suggested alternative configuration).
E.g., if we take a look at the configuration example:

  • GetCapabilities supports: kvp, xml, soap
  • GetFeature supports: kvp, xml
  • DescribeFeatureType supports: kvp
  • All other request types support: kvp

@dstenger
Copy link
Contributor Author

dstenger commented Jul 6, 2015

We implemented following configuration concept as proposed by @copierrj:

  <SupportedRequests>
    <SupportedEncodings>kvp</SupportedEncodings>
    <GetCapabilities>
      <SupportedEncodings>xml soap</SupportedEncodings>
    </GetCapabilities>
    <DescribeFeatureType/>
    <GetFeature>
      <SupportedEncodings>xml</SupportedEncodings>
    </GetFeature>
  </SupportedRequests>

A few examples how the new configuration concept works are pictured below (including some tricky ones):

  <SupportedRequests>
    <GetCapabilities>
      <SupportedEncodings>kvp</SupportedEncodings>
    </GetCapabilities>
    <DescribeFeatureType>
      <SupportedEncodings>xml</SupportedEncodings>
    </DescribeFeatureType>
  </SupportedRequests>

--> Just GetCapabilities with KVP encoding and DescribeFeatureType with XML encoding work

  <SupportedRequests>
    <SupportedEncodings>kvp</SupportedEncodings>
  </SupportedRequests>

--> All requests with KVP encoding are supported. As there does not exist any request configuration but a global SupportedEncodings configuration no request is excluded.

  <SupportedRequests>
  </SupportedRequests>

--> All requests with all encodings are supported. As there does not exist any request configuration or SupportedEncodings configuration no request/encoding is excluded.

  <SupportedRequests>
    <SupportedEncodings>kvp</SupportedEncodings>
    <GetCapabilities/>
    <GetFeature/>
  </SupportedRequests>

--> GetCapabilities and GetFeature with KVP encoding are supported.

  <SupportedRequests>
    <GetCapabilities/>
    <GetFeature/>
  </SupportedRequests>

--> GetCapabilities and GetFeature with all encodings are supported. As there does not exist any SupportedEncodings configuration, no encoding is excluded.

  <SupportedRequests>
    <SupportedEncodings>kvp</SupportedEncodings>
    <GetCapabilities>
      <SupportedEncodings>xml soap</SupportedEncodings>
    </GetCapabilities>
    <DescribeFeatureType/>
    <GetFeature>
      <SupportedEncodings>xml</SupportedEncodings>
    </GetFeature>
  </SupportedRequests>

--> This is the previous discussed example. GetCapabilities with KVP, XML and SOAP encoding, DescribeFeatureType with KVP encoding and GetFeature with KVP and XML encoding are supported.

lgoltz added 3 commits July 6, 2015 11:02
…xact matching; fixed implementation for empty SupportedRequest and global supported encodings configuration
copierrj added a commit that referenced this pull request Oct 1, 2015
Supported request encodings are made configurable for WFS
@copierrj copierrj merged commit 6ae7554 into deegree:master Oct 1, 2015
@tfr42 tfr42 removed the needs discussion requires discussion with contributor label Oct 1, 2015
@lgoltz lgoltz deleted the wfsMethodsConfigurable-1995 branch December 8, 2022 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants