Re: SLING-11974: Spec Compliance vs. Backward compatibility
I added the switch. If anyone wants improvements to this, please directly commit or PR Thanks Carsten On 21.07.2023 06:32, Carsten Ziegeler wrote: Sure, we can make this configurable. Nevertheless, I strongly suggest everyone to not rely on this method returning null for the anonymous case and rather use the other two methods - which always behaved spec compliant. Otherwise you might run into trouble once you combine two sources of code where one is expecting the spec compliant behaviour and the other one is not. From experience, this will sooner or later happen Regards Carsten On 20.07.2023 19:20, Eric Norman wrote: Carsten, unfortunately, it seems that the problem is more complicated than how you have described it. There have been 2 public releases of org.apache.sling.engine with the fix from SLING-11825 included. People (including me) have already migrated to those releases and made changes to their code to compensate for that difference and if you change it back to the previous way then we have to revert those changes again. I shouldn't have to revert spec compliant code to be bug compatible with someone else's old (and wrong) code. Why can't you just temporarily make the behavior configurable in the near term? Default to the #2 behavior if that is the most common scenario and then declare that the old behavior is deprecated. If the configuration resolves to #2 then simply log a warning about usage of the non-spec behavior and indicate that the wrong behavior may be removed in some future release. This log message and perhaps some hints in the README or other documentation can nudge the community toward changing their code to use the spec compliant behavior. Regards, -Eric On Thu, Jul 20, 2023 at 3:28 AM Konrad Windszus wrote: Hi, Carsten just reverted the fix from https://issues.apache.org/jira/browse/SLING-11825 in https://issues.apache.org/jira/browse/SLING-11974. The fix is correct according to the Servlet Spec, but it seems some customer rely on Sling behaving not spec compliant here. The question is what weighs more: 1) Spec compliance to make it easier for most new/existing users as otherwise behaviour differs from Javadoc and underlying Spec. 2) Backwards compatibility for those users who rely on this spec incompatibility. In my opinion I would clearly go for 1) but I would like to hear other opinions. Thanks, Konrad -- Carsten Ziegeler Adobe cziege...@apache.org
Re: SLING-11974: Spec Compliance vs. Backward compatibility
Sure, we can make this configurable. Nevertheless, I strongly suggest everyone to not rely on this method returning null for the anonymous case and rather use the other two methods - which always behaved spec compliant. Otherwise you might run into trouble once you combine two sources of code where one is expecting the spec compliant behaviour and the other one is not. From experience, this will sooner or later happen Regards Carsten On 20.07.2023 19:20, Eric Norman wrote: Carsten, unfortunately, it seems that the problem is more complicated than how you have described it. There have been 2 public releases of org.apache.sling.engine with the fix from SLING-11825 included. People (including me) have already migrated to those releases and made changes to their code to compensate for that difference and if you change it back to the previous way then we have to revert those changes again. I shouldn't have to revert spec compliant code to be bug compatible with someone else's old (and wrong) code. Why can't you just temporarily make the behavior configurable in the near term? Default to the #2 behavior if that is the most common scenario and then declare that the old behavior is deprecated. If the configuration resolves to #2 then simply log a warning about usage of the non-spec behavior and indicate that the wrong behavior may be removed in some future release. This log message and perhaps some hints in the README or other documentation can nudge the community toward changing their code to use the spec compliant behavior. Regards, -Eric On Thu, Jul 20, 2023 at 3:28 AM Konrad Windszus wrote: Hi, Carsten just reverted the fix from https://issues.apache.org/jira/browse/SLING-11825 in https://issues.apache.org/jira/browse/SLING-11974. The fix is correct according to the Servlet Spec, but it seems some customer rely on Sling behaving not spec compliant here. The question is what weighs more: 1) Spec compliance to make it easier for most new/existing users as otherwise behaviour differs from Javadoc and underlying Spec. 2) Backwards compatibility for those users who rely on this spec incompatibility. In my opinion I would clearly go for 1) but I would like to hear other opinions. Thanks, Konrad -- Carsten Ziegeler Adobe cziege...@apache.org
Re: SLING-11974: Spec Compliance vs. Backward compatibility
Carsten, unfortunately, it seems that the problem is more complicated than how you have described it. There have been 2 public releases of org.apache.sling.engine with the fix from SLING-11825 included. People (including me) have already migrated to those releases and made changes to their code to compensate for that difference and if you change it back to the previous way then we have to revert those changes again. I shouldn't have to revert spec compliant code to be bug compatible with someone else's old (and wrong) code. Why can't you just temporarily make the behavior configurable in the near term? Default to the #2 behavior if that is the most common scenario and then declare that the old behavior is deprecated. If the configuration resolves to #2 then simply log a warning about usage of the non-spec behavior and indicate that the wrong behavior may be removed in some future release. This log message and perhaps some hints in the README or other documentation can nudge the community toward changing their code to use the spec compliant behavior. Regards, -Eric On Thu, Jul 20, 2023 at 3:28 AM Konrad Windszus wrote: > Hi, > Carsten just reverted the fix from > https://issues.apache.org/jira/browse/SLING-11825 in > https://issues.apache.org/jira/browse/SLING-11974. > The fix is correct according to the Servlet Spec, but it seems some > customer rely on Sling behaving not spec compliant here. > The question is what weighs more: > 1) Spec compliance to make it easier for most new/existing users as > otherwise behaviour differs from Javadoc and underlying Spec. > 2) Backwards compatibility for those users who rely on this spec > incompatibility. > > > In my opinion I would clearly go for 1) but I would like to hear other > opinions. > > Thanks, > Konrad
Re: SLING-11974: Spec Compliance vs. Backward compatibility
Good point, I updated them https://github.com/apache/sling-org-apache-sling-api/commit/b76ab7e07c79dab4cd89eb25784848c2f5ad2732 Regards Carsten On 20.07.2023 15:22, Robert Munteanu wrote: +/** + * Returns a java.security.Principal object containing + * the name of the current authenticated user. + * + * Note: This method deviates from the parent interface and never returns null, + * even whenthe user is not authenticated. + * + * @return a java.security.Principal containing + * the name of the user making this request; never + * null + */ +@NotNull +Principal getUserPrincipal(); -- Carsten Ziegeler Adobe cziege...@apache.org
Re: SLING-11974: Spec Compliance vs. Backward compatibility
On Thu, 2023-07-20 at 14:34 +0200, Carsten Ziegeler wrote:
> Sure, the question is where?
>
> I looked at our existing docs, and we actually document how to check
> for
> anonymous access. But that is a little bit hidden, embedded in
> outdated
> docs
We can start with the javadoc of the interface, something like
diff --git
a/src/main/java/org/apache/sling/api/SlingHttpServletRequest.java
b/src/main/java/org/apache/sling/api/SlingHttpServletRequest.java
index 776e613..fa32b59 100644
--- a/src/main/java/org/apache/sling/api/SlingHttpServletRequest.java
+++ b/src/main/java/org/apache/sling/api/SlingHttpServletRequest.java
@@ -18,6 +18,7 @@
*/
package org.apache.sling.api;
+import java.security.Principal;
import java.util.Enumeration;
import java.util.List;
import java.util.Locale;
@@ -274,4 +275,18 @@ public interface SlingHttpServletRequest extends
HttpServletRequest, Adaptable {
* @return The request progress tracker.
*/
@NotNull RequestProgressTracker getRequestProgressTracker();
+
+/**
+ * Returns a java.security.Principal object
containing
+ * the name of the current authenticated user.
+ *
+ * Note: This method deviates from the parent
interface and never returns null,
+ * even whenthe user is not authenticated.
+ *
+ * @return a java.security.Principal
containing
+ * the name of the user making this request;
never
+ * null
+ */
+@NotNull
+Principal getUserPrincipal();
}
>
> Regards
> Carsten
>
> On 20.07.2023 13:06, Jörg Hoh wrote:
> > Should we document that in this case we are not spec compliant for
> > backwards compatibility reasons?
> >
> > Am Do., 20. Juli 2023 um 12:53 Uhr schrieb Carsten Ziegeler <
> > cziege...@apache.org>:
> >
> > > I think there is no one solution fits all here. As always it
> > > depends.
> > >
> > > In general we should try to be spec compliant - unless there is a
> > > good
> > > reason not to. There could be different reasons.
> > >
> > > In this particular case, imho there is a good reason to not be
> > > compliant. We have a huge user base and the non spec compliant
> > > behaviour
> > > is in there for many many years. There is a chance that some of
> > > our
> > > users rely on this behaviour. If we change it, we break our
> > > users. Which
> > > actually happened in this case.
> > >
> > > In addition, in this case if users are trying out our non spec
> > > compliant
> > > method they will immediately see that it is not compliant during
> > > development/testing.
> > >
> > > Regards
> > > Carsten
> > >
> > > On 20.07.2023 12:28, Konrad Windszus wrote:
> > > > Hi,
> > > > Carsten just reverted the fix from
> > > https://issues.apache.org/jira/browse/SLING-11825 in
> > > https://issues.apache.org/jira/browse/SLING-11974.
> > > > The fix is correct according to the Servlet Spec, but it seems
> > > > some
> > > customer rely on Sling behaving not spec compliant here.
> > > > The question is what weighs more:
> > > > 1) Spec compliance to make it easier for most new/existing
> > > > users as
> > > otherwise behaviour differs from Javadoc and underlying Spec.
> > > > 2) Backwards compatibility for those users who rely on this
> > > > spec
> > > incompatibility.
> > > >
> > > >
> > > > In my opinion I would clearly go for 1) but I would like to
> > > > hear other
> > > opinions.
> > > >
> > > > Thanks,
> > > > Konrad
> > >
> > > --
> > > Carsten Ziegeler
> > > Adobe
> > > cziege...@apache.org
> > >
> >
> >
>
Re: SLING-11974: Spec Compliance vs. Backward compatibility
Hi, yes I'm heavily opting for 2) :) you know I initially approved your PR thinking that this should not break any of Sling's users. Well, today I know that this assumption was wrong. I know of several users which currently rely on the wrong behaviour - and changing it breaks them. The longer a behaviour exists the more likely it is used by someone. As there are other means to check for an anonymous request and we have at least one documented and the other one used throughout the code base, I think it is in this case more important to not break our users. Of course as Jörg pointed out we should somehow document this. Regards Carsten On 20.07.2023 14:37, Konrad Windszus wrote: On 20. Jul 2023, at 12:53, Carsten Ziegeler wrote: I think there is no one solution fits all here. As always it depends. Yes, I agree with that. I was referring to this specific use case. In general we should try to be spec compliant - unless there is a good reason not to. There could be different reasons. In this particular case, imho there is a good reason to not be compliant. We have a huge user base and the non spec compliant behaviour is in there for many many years. There is a chance that some of our users rely on this behaviour. If we change it, we break our users. Which actually happened in this case. The argument for how long a bug does exist does not matter to much to me TBH, let us always strive to improve/fix things. Also if we break some customer this does not necessarily warrant that we don’t fix stuff, if the custom code in this case if clearly relying on spec incompatible behaviour. In addition, in this case if users are trying out our non spec compliant method they will immediately see that it is not compliant during development/testing. Not necessarily this is caught during tests (although I agree it should be). I stumbled upon this and it costed me some time to figure this out. Also this will be reported otherwise again (for good reasons) But I see, that you are opting for 2) in this case :-) Konrad Regards Carsten On 20.07.2023 12:28, Konrad Windszus wrote: Hi, Carsten just reverted the fix from https://issues.apache.org/jira/browse/SLING-11825 in https://issues.apache.org/jira/browse/SLING-11974. The fix is correct according to the Servlet Spec, but it seems some customer rely on Sling behaving not spec compliant here. The question is what weighs more: 1) Spec compliance to make it easier for most new/existing users as otherwise behaviour differs from Javadoc and underlying Spec. 2) Backwards compatibility for those users who rely on this spec incompatibility. In my opinion I would clearly go for 1) but I would like to hear other opinions. Thanks, Konrad -- Carsten Ziegeler Adobe cziege...@apache.org -- Carsten Ziegeler Adobe cziege...@apache.org
Re: SLING-11974: Spec Compliance vs. Backward compatibility
> On 20. Jul 2023, at 12:53, Carsten Ziegeler wrote: > > I think there is no one solution fits all here. As always it depends. Yes, I agree with that. I was referring to this specific use case. > > In general we should try to be spec compliant - unless there is a good reason > not to. There could be different reasons. > > In this particular case, imho there is a good reason to not be compliant. We > have a huge user base and the non spec compliant behaviour is in there for > many many years. There is a chance that some of our users rely on this > behaviour. If we change it, we break our users. Which actually happened in > this case. The argument for how long a bug does exist does not matter to much to me TBH, let us always strive to improve/fix things. Also if we break some customer this does not necessarily warrant that we don’t fix stuff, if the custom code in this case if clearly relying on spec incompatible behaviour. > > In addition, in this case if users are trying out our non spec compliant > method they will immediately see that it is not compliant during > development/testing. Not necessarily this is caught during tests (although I agree it should be). I stumbled upon this and it costed me some time to figure this out. Also this will be reported otherwise again (for good reasons) But I see, that you are opting for 2) in this case :-) Konrad > > Regards > Carsten > > On 20.07.2023 12:28, Konrad Windszus wrote: >> Hi, >> Carsten just reverted the fix from >> https://issues.apache.org/jira/browse/SLING-11825 in >> https://issues.apache.org/jira/browse/SLING-11974. >> The fix is correct according to the Servlet Spec, but it seems some customer >> rely on Sling behaving not spec compliant here. >> The question is what weighs more: >> 1) Spec compliance to make it easier for most new/existing users as >> otherwise behaviour differs from Javadoc and underlying Spec. >> 2) Backwards compatibility for those users who rely on this spec >> incompatibility. >> In my opinion I would clearly go for 1) but I would like to hear other >> opinions. >> Thanks, >> Konrad > > -- > Carsten Ziegeler > Adobe > cziege...@apache.org
Re: SLING-11974: Spec Compliance vs. Backward compatibility
Sure, the question is where? I looked at our existing docs, and we actually document how to check for anonymous access. But that is a little bit hidden, embedded in outdated docs Regards Carsten On 20.07.2023 13:06, Jörg Hoh wrote: Should we document that in this case we are not spec compliant for backwards compatibility reasons? Am Do., 20. Juli 2023 um 12:53 Uhr schrieb Carsten Ziegeler < cziege...@apache.org>: I think there is no one solution fits all here. As always it depends. In general we should try to be spec compliant - unless there is a good reason not to. There could be different reasons. In this particular case, imho there is a good reason to not be compliant. We have a huge user base and the non spec compliant behaviour is in there for many many years. There is a chance that some of our users rely on this behaviour. If we change it, we break our users. Which actually happened in this case. In addition, in this case if users are trying out our non spec compliant method they will immediately see that it is not compliant during development/testing. Regards Carsten On 20.07.2023 12:28, Konrad Windszus wrote: Hi, Carsten just reverted the fix from https://issues.apache.org/jira/browse/SLING-11825 in https://issues.apache.org/jira/browse/SLING-11974. The fix is correct according to the Servlet Spec, but it seems some customer rely on Sling behaving not spec compliant here. The question is what weighs more: 1) Spec compliance to make it easier for most new/existing users as otherwise behaviour differs from Javadoc and underlying Spec. 2) Backwards compatibility for those users who rely on this spec incompatibility. In my opinion I would clearly go for 1) but I would like to hear other opinions. Thanks, Konrad -- Carsten Ziegeler Adobe cziege...@apache.org -- Carsten Ziegeler Adobe cziege...@apache.org
Re: SLING-11974: Spec Compliance vs. Backward compatibility
Should we document that in this case we are not spec compliant for backwards compatibility reasons? Am Do., 20. Juli 2023 um 12:53 Uhr schrieb Carsten Ziegeler < cziege...@apache.org>: > I think there is no one solution fits all here. As always it depends. > > In general we should try to be spec compliant - unless there is a good > reason not to. There could be different reasons. > > In this particular case, imho there is a good reason to not be > compliant. We have a huge user base and the non spec compliant behaviour > is in there for many many years. There is a chance that some of our > users rely on this behaviour. If we change it, we break our users. Which > actually happened in this case. > > In addition, in this case if users are trying out our non spec compliant > method they will immediately see that it is not compliant during > development/testing. > > Regards > Carsten > > On 20.07.2023 12:28, Konrad Windszus wrote: > > Hi, > > Carsten just reverted the fix from > https://issues.apache.org/jira/browse/SLING-11825 in > https://issues.apache.org/jira/browse/SLING-11974. > > The fix is correct according to the Servlet Spec, but it seems some > customer rely on Sling behaving not spec compliant here. > > The question is what weighs more: > > 1) Spec compliance to make it easier for most new/existing users as > otherwise behaviour differs from Javadoc and underlying Spec. > > 2) Backwards compatibility for those users who rely on this spec > incompatibility. > > > > > > In my opinion I would clearly go for 1) but I would like to hear other > opinions. > > > > Thanks, > > Konrad > > -- > Carsten Ziegeler > Adobe > cziege...@apache.org > -- Cheers, Jörg Hoh, https://cqdump.joerghoh.de Twitter: @joerghoh
Re: SLING-11974: Spec Compliance vs. Backward compatibility
I think there is no one solution fits all here. As always it depends. In general we should try to be spec compliant - unless there is a good reason not to. There could be different reasons. In this particular case, imho there is a good reason to not be compliant. We have a huge user base and the non spec compliant behaviour is in there for many many years. There is a chance that some of our users rely on this behaviour. If we change it, we break our users. Which actually happened in this case. In addition, in this case if users are trying out our non spec compliant method they will immediately see that it is not compliant during development/testing. Regards Carsten On 20.07.2023 12:28, Konrad Windszus wrote: Hi, Carsten just reverted the fix from https://issues.apache.org/jira/browse/SLING-11825 in https://issues.apache.org/jira/browse/SLING-11974. The fix is correct according to the Servlet Spec, but it seems some customer rely on Sling behaving not spec compliant here. The question is what weighs more: 1) Spec compliance to make it easier for most new/existing users as otherwise behaviour differs from Javadoc and underlying Spec. 2) Backwards compatibility for those users who rely on this spec incompatibility. In my opinion I would clearly go for 1) but I would like to hear other opinions. Thanks, Konrad -- Carsten Ziegeler Adobe cziege...@apache.org
