Issue #12806 - Overhaul ComplianceViolation.Listener reporting

[joakime]

Cleanup all code paths that perform a ComplianceViolation check to do the following ...

• Always report the violation, if detected.
• Report if the violation was allowed (or not)
• Use the HttpChannelState.getCompliaceViolationListener() in all cases (this gets the benefit of a single ComplianceViolation.Listener to use, along with all of the initialize(), onRequestBegin(), and onRequestEnd() techniques.
• Deprecate the HttpConfiguration notify methods
• Deprecate the ComplianceViolation.notify() static methods.

Closes #12806

[gregw]

Looking better, but still work to do.

[gregw]

This variable is not used

[gregw]

should we assertNotNull on the cVL? or skip this line if it is null?

[joakime]

The expectation is that there will always be a cVL, even if its just ComplianceViolation.Listener.NOOP.
I added a requireNonNull to the start of the method anyway.

[gregw]

javadoc

[joakime]

Easier to just get rid of this method, it's no longer being used anyway.

[gregw]

never called?

[joakime]

Removed

[gregw]

javadoc

[gregw]

Also, would this not be better as a method on HttpConfiguration? When would a channel ever have a different impl?

[joakime]

Each channel has a (potentially) unique set of cVL due cVL.initialize() requirement.
The HttpConfiguration approach is the wrong place for most use cases with cVL.
It only really fits (and barely at that) for pre-channel violations like from HttpParser (but even that isn't really a concern anymore with the channel based approach).

[joakime]

javadoc added

[gregw]

This method is substantially the same as the one above. Factor out the mode selection maybe to HttpConfiguration itself

[joakime]

Removed

[gregw]

I think it is more natural to report violations to the HttpConfiguration rather than the HttpChannel

[joakime]

But that's the wrong place and doesn't take into account the cVL.initialize() / cVL.onRequestBegin() / cVL.onRequestEnd() requirements.
The HttpChannel is also the place where these events fire.
The individual violations (cVL.onComplianceViolation()) occur between these.

[gregw]

The mode selection code is duplicated again. Perhaps we should just have the following methods on HttpConfiguration:

    public ComplianceViolation.Mode getComplianceViolationModeFor(ComplianceViolation violation)
    {
        if (violation instanceof UriCompliance.Violation)
            return getUriCompliance();
        else if (violation instanceof HttpCompliance.Violation)
            return getHttpCompliance();
        else if (violation instanceof MultiPartCompliance.Violation)
            return getMultiPartCompliance();
        else if (violation instanceof CookieCompliance.Violation)
            return getRequestCookieCompliance();
        else
            throw new UnsupportedOperationException("Unsupported ComplianceViolation type: " + violation.getClass().getName());
    }

    public <T extends Throwable> void assertAllowed(ComplianceViolation violation, String detail, Function<String, T> error) throws T
    {
        ComplianceViolation.Mode mode = getComplianceViolationModeFor(violation);
        boolean allowed = mode.allows(violation);

        List<ComplianceViolation.Listener> listeners = getComplianceViolationListeners();
        ComplianceViolation.Event event = new ComplianceViolation.Event(mode, violation, detail, allowed);
        Throwable throwable = null;
        for (ComplianceViolation.Listener listener : listeners)
        {
            try
            {
                listener.onComplianceViolation(event);
            }
            catch (Throwable t)
            {
                throwable = ExceptionUtil.combine(throwable, t);
            }
        }
        ExceptionUtil.ifExceptionThrowUnchecked(throwable);

        if (!allowed)
            throw error.apply(violation.getDescription());
    }

[joakime]

Removed, no longer being used.