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

OmniPartialResponseWriter: missing startDocument() #426

Closed
mmariotti opened this Issue Dec 22, 2017 · 1 comment

Comments

Projects
None yet
2 participants
@mmariotti
Contributor

mmariotti commented Dec 22, 2017

Hello,
I'm enhancing my ExceptionHandler and I'm trying to use my own PartialResponseWriter to track startDocument() and endDocument() calls.

In combination with Omnifaces, my startDocument() is never called: this happens because OmniPartialResponseWriter does not override the method and the super implementation is called, skipping all other wrapped writers (I'm on mojarra 2.2.15).

So, I'm asking to add this override.

Also there are two other small things:

  1. please, can you add a isUpdating() method? This is useful when dealing with committed responses where we don't know what is the actual content of the response buffer and we can't reset it.
    I'm asking for this because in my ExceptionHandler I'm trying to perform a final "desperate" redirect and I don't know if I have to call endUpdate(), see some example code:
else if(ajax && committed)
{
	String url = ...

	XXXPartialResponseWriter writer = getXXXPartialResponseWriter();

	if(writer.isUpdating())
	{
		writer.endUpdate();
		writer.startError("");
		writer.endError();
	}
	else if(!writer.isDocumentStarted())
	{
		writer.startDocument();
	}

	writer.redirect(url);
	writer.endDocument();
}
  1. Last thing, I see that updating flag is not reset here:
public void reset() {
	try {
		wrapped.flush(); // Note: this doesn't actually flush to writer, but clears internal state.

		if (updating) {
			// If reset() method is entered with updating=true, then it means that Mojarra is used and that
			// an exception was been thrown during ajax render response. The following calls will gently close
			// the partial response which Mojarra has left open.
			// MyFaces never enters reset() method with updating=true, this is handled in endDocument() method.
			wrapped.startError("");
			wrapped.endError();
			wrapped.endElement("partial-response"); // Don't use endDocument() as it will flush.
		}
	}
	catch (IOException e) {
		throw new FacesException(e);
	}
	finally {
		responseReset();
	}
}

is this wanted, or is it a mistake?

Thank you and happy holidays!

@BalusC

This comment has been minimized.

Show comment
Hide comment
@BalusC

BalusC Dec 22, 2017

Member

I fixed the missing startDocument() and reset of updating. It'll be in 2.6.8-SNAPSHOT and 3.0-RC2.

I won't add an isUpdating() as 1) OmniPartialResponseWriter impl is deliberately private and 2) this isn't in PartialResposneWriter API. Better keep track yourself in start/endUpdate() of your PartialResponseWriter.

Member

BalusC commented Dec 22, 2017

I fixed the missing startDocument() and reset of updating. It'll be in 2.6.8-SNAPSHOT and 3.0-RC2.

I won't add an isUpdating() as 1) OmniPartialResponseWriter impl is deliberately private and 2) this isn't in PartialResposneWriter API. Better keep track yourself in start/endUpdate() of your PartialResponseWriter.

@BalusC BalusC closed this Dec 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment