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

HPCC-32948 Make ws_logaccess WSDL match exception return behavior #19301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

asselitx
Copy link
Contributor

@asselitx asselitx commented Nov 14, 2024

The service returns exceptions inline, but the WSDL does not show the exceptions array in the method response elements.

Fix the interface defn to ensure the WSDL shows exceptions are returned inline.

Note these caveats:

  • This change cannot be versioned and will retroactively change the WSDLs for all service interface versions. The alternative is that a current exception response would not validate against the WSDL.
  • Even with the fix the response will not match the WSDL in some cases. Transactions sent with "Content Type: application/x-www-form-urlencoded" (most of the requests from ECLWatch) return inline exception responses missing the root "ESPresponse" element. The root of the response is the "Exceptions" element.

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

Copy link

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32948

Jirabot Action Result:
Workflow Transition To: Merge Pending
Updated PR

@asselitx
Copy link
Contributor Author

@rpastrana I wanted you to be aware of this to ensure it wouldn't disrupt any of your java interfaces. Also, I've targeted master because this would retroactively adjust WSDLs without versioning, but I can retarget if appropriate.

Copy link
Member

@rpastrana rpastrana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asselitx thanks for the headsup, fortunately hpcc4j doesn't yet support this service and will be unaffected by your change.
As far as the code review, there's a couple of added superfluous new-lines which should be reverted

The service returns exceptions inline, but the WSDL does not show the
exceptions array in the method response elements.

Fix the interface defn to ensure the WSDL shows exceptions are returned
inline.

Note these caveats:

- This change cannot be versioned and will retroactively change
the WSDLs for all service interface versions. The alternative is that a
current exception response would not validate against the WSDL.
- Even with the fix the response will not match the WSDL in some cases.
Transactions sent with "Content Type: application/x-www-form-urlencoded"
(most of the requests from ECLWatch) return inline exception responses
missing the root "ESPresponse" element. The root of the response is the
"Exceptions" element.

Signed-off-by: Terrence Asselin <terrence.asselin@lexisnexisrisk.com>
@asselitx asselitx force-pushed the logaccess-exceptions-hpcc-32948 branch from eb3e8ff to a433f9b Compare November 15, 2024 15:18
@rpastrana
Copy link
Member

@asselitx one quick question, should we bump the service version to account for the new structure?

@asselitx
Copy link
Contributor Author

@asselitx one quick question, should we bump the service version to account for the new structure?

I don't think that would be helpful, but we're kind of painted into a corner without a real elegant solution.

There's no way to anchor the start of using exceptions_inline to a version with the esdl engine as its written now. Adding that property retroactively affects all versions, so it didn't seem helpful to increment. Since all versions of the service are currently "out of compliance" and would not validate against the WSDL for any exception returns, it seemed reasonable to make the WSDL match behavior that other clients may already be expecting/compensating for.

Other possible approaches would these, but neither would fix the current methods:

  1. Deprecate the two current methods and create two new methods on an incremented version with the correct property that makes the WSDL match behavior.
  2. Change the esdl engine to support versioning of properties, and use that new feature to make the WSDL match behavior starting on a new service version.

@rpastrana
Copy link
Member

rpastrana commented Nov 19, 2024

@asselitx one quick question, should we bump the service version to account for the new structure?

I don't think that would be helpful, but we're kind of painted into a corner without a real elegant solution.

There's no way to anchor the start of using exceptions_inline to a version with the esdl engine as its written now. Adding that property retroactively affects all versions, so it didn't seem helpful to increment. Since all versions of the service are currently "out of compliance" and would not validate against the WSDL for any exception returns, it seemed reasonable to make the WSDL match behavior that other clients may already be expecting/compensating for.

Other possible approaches would these, but neither would fix the current methods:

1. Deprecate the two current methods and create two new methods on an incremented version with the correct property that makes the WSDL match behavior.

2. Change the esdl engine to support versioning of properties, and use that new feature to make the WSDL match behavior starting on a new service version.

@asselitx thanks for clarifying. As far as wslogaccess, eclwatch is our only known WSDL consumer. We'll have to get @jeclrsg 's take on this change, but I don't think option 1 is worthwhile.

If 'exceptions_inline' is the new expected standard, can we default it in for all new methods?

The 2nd option seems like a sensible feature in general.

@asselitx
Copy link
Contributor Author

  1. Change the esdl engine to support versioning of properties, and use that new feature to make the WSDL match behavior starting on a new service version.

@asselitx thanks for clarifying. As far as wslogaccess, eclwatch is our only known WSDL consumer. We'll have to get @jeclrsg 's take on this change, but I don't think option 1 is worthwhile.

If 'exceptions_inline' is the new expected standard, can we default it in for all new methods?

The 2nd option seems like a sensible feature in general.

I've created HPCC-33035 to more immediately address the hole that made this mismatch possible, and which will make it much simpler to add exceptions inline to all methods in a service. Making the option #2 is change would be a bigger effort I'd want to clear with Gavin before starting.

For the short term HPCC-32992 lists all other services that should be fixed in the same way we're fixing ws_logaccess, and has links to related tickets.

@asselitx
Copy link
Contributor Author

@jeclrsg Do you see any issues with this change? The WSDL/Schema for ws_logaccess will now show exceptions inline for both methods, and this change will be effective for all service versions. This will be making the WSDL match the behavior you have always seen from the service.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants