Skip to main content
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
Re: [jsp-dev] Discussion on deprecating the ErrorData constructor without a queryString parameter
I have the PR up and ready for review: https://github.com/jakartaee/pages/pull/251

Thanks,

Paul Nicolucci

On Wed, Nov 8, 2023 at 9:48 AM Paul Nicolucci <pnicolucci@xxxxxxxxx> wrote:
I can get a PR up to revert the changes from https://github.com/jakartaee/pages/pull/250

I'll post to this thread once I have it ready for review.

Regards,

Paul Nicolucci

On Wed, Nov 8, 2023 at 9:44 AM Volodymyr Siedlecki via jsp-dev <jsp-dev@xxxxxxxxxxx> wrote:
Hi,

I agree with Mark.  It's probably best to deprecate and remove the old constructor. ErrorData looks to be used only within the API, not implementations (1) nor applications.
Developers would only retrieve the query string, and not create errorData instances themselves.

1) Searched within WaSP and Open Liberty

Best,
Volodymyr

On Mon, Oct 30, 2023 at 9:13 AM Paul Nicolucci via jsp-dev <jsp-dev@xxxxxxxxxxx> wrote:

My initial thinking was if someone was extending ErrorData and overriding PageContext.getErrorData() but that doesn't look to be a valid concern.

So I think the only argument against the deprecation and eventual removal would be if an application/container were using the old Constructor of ErrorData.

Regards,

Paul Nicolucci

On Wed, Oct 25, 2023 at 5:32 AM Mark Thomas via jsp-dev <jsp-dev@xxxxxxxxxxx> wrote:
Apologies for the flip-flopping but on further reflection I think there
are good reasons to keep this deprecation.

The old constructor was only used by the API and is no longer used (the
API now uses the new constructor).

It seems unlikely that a container or application would be using the old
constructor but if they are, they need to switch to using the new one
and the deprecation is a way of getting that message across.

While null is valid for the query string, the query string must be
provided if available. It boils down to:

With old constructor:

String qs = (String)
getRequest().getAttribute("jakarta.servlet.error.query_string");
if (qs == null) {
     return new ErrorData((Throwable)
getRequest().getAttribute("jakarta.servlet.error.exception"),
             ((Integer)
getRequest().getAttribute("jakarta.servlet.error.status_code")).intValue(),
             (String)
getRequest().getAttribute("jakarta.servlet.error.request_uri"),
             (String)
getRequest().getAttribute("jakarta.servlet.error.servlet_name"));
} else {
     return new ErrorData((Throwable)
getRequest().getAttribute("jakarta.servlet.error.exception"),
             ((Integer)
getRequest().getAttribute("jakarta.servlet.error.status_code")).intValue(),
             (String)
getRequest().getAttribute("jakarta.servlet.error.request_uri"),
             (String)
getRequest().getAttribute("jakarta.servlet.error.servlet_name"),
             (String)
getRequest().getAttribute("jakarta.servlet.error.query_string"));
}


With just the new constructor:

return new ErrorData((Throwable)
getRequest().getAttribute("jakarta.servlet.error.exception"),
         ((Integer)
getRequest().getAttribute("jakarta.servlet.error.status_code")).intValue(),
         (String)
getRequest().getAttribute("jakarta.servlet.error.request_uri"),
         (String)
getRequest().getAttribute("jakarta.servlet.error.servlet_name"),
         (String)
getRequest().getAttribute("jakarta.servlet.error.query_string"));


I can't think of any scenario where it helps to keep the old
constructor. I'll give it another week or so for folks to comment but my
intention at this point is to revert the removal of the deprecation.

Mark



On 14/10/2023 23:37, Mark Thomas via jsp-dev wrote:
> On 09/10/2023 11:07, Paul Nicolucci via jsp-dev wrote:
>
>> Why don't we keep both constructors if null is perfectly valid for the
>> queryString?
>
> Good question. I was probably thinking in terms of replacing the old
> constructor with the new one.
>
> Since queryString looks to be the only optional value passed to
> ErrorData, I can see the benefit in keeping the existing constructor.
>
> If no other opinions are expressed, we can remove the deprecation in a
> week or so.
>
> Mark
> _______________________________________________
> jsp-dev mailing list
> jsp-dev@xxxxxxxxxxx
> To unsubscribe from this list, visit
> https://www.eclipse.org/mailman/listinfo/jsp-dev
_______________________________________________
jsp-dev mailing list
jsp-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsp-dev
_______________________________________________
jsp-dev mailing list
jsp-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsp-dev
_______________________________________________
jsp-dev mailing list
jsp-dev@xxxxxxxxxxx
To unsubscribe from this list, visit https://www.eclipse.org/mailman/listinfo/jsp-dev

Back to the top