RE: [RTC] ThreadLocal for getting current request in sling
hello justin. >> it is an implementation details of the post processing business models that >they need a request for some part of their job (e.g. distinguish between edit >and preview mode). the caller of the initial adaption is not necessary aware >of this and should not care of it in my point of view. > >This is all just an implementation choice. Nothing is forcing you do >to the post processing in an object which doesn't have the request. >You can just as easily (probably better) do that post processing in an >object which has direct access to the request. Or, as Alex suggested, >provide the request to those objects once they are adapted. > >And of course the caller has to be aware that the request is >necessary. It is part of the context of that the execution of that >post processing. in our current approach the post processing step should not be separate, and the user of the models should not be aware of it. but we are still in the first few months of developing our new architecture style based on sling models, perhaps we think otherwise a few months ahead when the first applications with it are finished. it is too early for me to make a final judgement. coming back to my summary post [1] I have the feeling we should go with A) currently, and see if others have the same needs for such a threadlocal in the future - or not. unit then i can continue experiment with it outside the main sling build. stefan [1] http://apache-sling.73963.n3.nabble.com/RE-RTC-ThreadLocal-for-getting-current-request-in-sling-summary-tt4042631.html > >> if you call and OSGi service you do not have to know what services this has >bound internally, OSGi takes care that all are resolved or the service is not >available. > >I don't think this is the right analogy. You aren't talking about >service dependencies, you are talking about the API. If I have a >method called getFoo(Object context) and the context object is >required for proper execution. If I pass null as the context object, >then I shouldn't expect to get a valid response. > >> >> i'm not a fan of the proposals to extend the adaptTo() semantic for this >usecase. not only because its difficult to do it without breaking existing >code (or leads to an interface like Adaptable2), the caller should not have to >now the implementation details for such cross-cutting concern models that are >required deeper in the model structure. for the same reason extending the >sling models factory interface or having additional "setRequest()" methods on >the models are no fitting solutions - the model that is adapted initially is >not the problem, but those further down the hierarchy. >> >> having an interface like "RequestScope" is basicall going into the right >direction - but who should detect this interface and inject the request? the >sling models implementation? than we have the same problem not able to geht >the request objects if it's not the adaptable. and sling models does not need >such interfaces, its solely based on @Inject annotations. > >As I said, this has nothing to do with Sling Models. It is a general >problem with adapters. > >Justin > >> >> this is getting a long thread, i'll write a summary to lead to a decision. >> >> stefan >>
Re: [RTC] ThreadLocal for getting current request in sling
Hi Dominik, It is true that the ClassUseProvider only deals with resource and request objects, but That's far from the only way to use adaptTo from Sightly scripts. Justin On Fri, Oct 24, 2014 at 10:40 AM, Dominik Süß wrote: > Hi Justin, > > it is not true that no change in sightly would be required sightly use just > tries to adapt resource or request, not any other objects (see > ClassUseProvider L65 ff as to be found in the contribution zip). > > I'm currently searching for the option to solve the case with the least > modifications and least impact but this is the constraint that I couldn't > eliminate. > > Cheers > Dominik > > On Fri, Oct 24, 2014 at 4:32 PM, Justin Edelson > wrote: > >> Hi Dominik, >> >> Anyone is free to define their own objects which implement Adaptable >> and their own model classes which can be adapted from those adaptable >> classes. This doesn't require any additions to any part of Sling nor >> any changes to Sightly. >> >> Regards, >> Justin >> >> On Fri, Oct 24, 2014 at 10:20 AM, Dominik Süß >> wrote: >> > Hey everybody, >> > >> > thinking about this again I also realized that changing the Adaptable >> > Interface or having another one creates more issues then solving. In >> fact >> > it would be way easier to have some generic Wrapperclass that can hold >> the >> > request and another object and implementing Adaptable. >> > >> > This would require some change for the Use semanticts of Sightly to wrap >> > request and resource (currentResource by default but could be overriden >> by >> > parameter) and the Models implementation would use this adaptable instead >> > of request _or_ resource. >> > >> > WDYT? >> > >> > Dominik >> > >> > On Fri, Oct 24, 2014 at 3:34 PM, Justin Edelson < >> jus...@justinedelson.com> >> > wrote: >> > >> >> Hi Stefan, >> >> >> >> On Fri, Oct 24, 2014 at 7:53 AM, Stefan Seifert > > >> >> wrote: >> >> > >> >> >>Personally, I don't quite see the use case -- if your AdapterFactory >> >> >>(whether it is generated by Sling Models or hand-coded) needs >> >> >>request-based context, the adaptable is the request. >> >> > >> >> > to shed some more light on my usecase an example: >> >> > >> >> > - a sightly template calls a "controller" model, which adapts from the >> >> current request >> >> > - this model wants to output an object structed based on a complex >> data >> >> structure (e.g. a list of nodes with subnodes) >> >> > - this data structure cannot be used directly for output, but some >> >> properties need some special processing >> >> > - so we build a set of models for this data structures adapting to a >> >> resource for each node, injecting each other for @ChildResource for the >> >> hierarchy >> >> > - each of this resource models does post processing on some >> attributes, >> >> e.g. >> >> > - resolving a media asset reference to a set of externalized image >> >> URLs for responsive imaging >> >> > - externalizing an URL respecting sling short URL mapping >> >> > - injecting dummy images or links for invalid references - but only >> if >> >> the edit mode of the CMS is active. >> >> > - this post processing is done with further models that implement the >> >> business logic to solve this centrally >> >> > >> >> > it is an implementation details of the post processing business models >> >> that they need a request for some part of their job (e.g. distinguish >> >> between edit and preview mode). the caller of the initial adaption is >> not >> >> necessary aware of this and should not care of it in my point of view. >> >> >> >> This is all just an implementation choice. Nothing is forcing you do >> >> to the post processing in an object which doesn't have the request. >> >> You can just as easily (probably better) do that post processing in an >> >> object which has direct access to the request. Or, as Alex suggested, >> >> provide the request to those objects once they are adapted. >> >> >> >> And of course the caller has to be aware that the request is >> >> necessary. It is part of the context of that the execution of that >> >> post processing. >> >> >> >> > if you call and OSGi service you do not have to know what services >> this >> >> has bound internally, OSGi takes care that all are resolved or the >> service >> >> is not available. >> >> >> >> I don't think this is the right analogy. You aren't talking about >> >> service dependencies, you are talking about the API. If I have a >> >> method called getFoo(Object context) and the context object is >> >> required for proper execution. If I pass null as the context object, >> >> then I shouldn't expect to get a valid response. >> >> >> >> > >> >> > i'm not a fan of the proposals to extend the adaptTo() semantic for >> this >> >> usecase. not only because its difficult to do it without breaking >> existing >> >> code (or leads to an interface like Adaptable2), the caller should not >> have >> >> to now the implementation details for such cross-cutting concern models >> >> that a
Re: [RTC] ThreadLocal for getting current request in sling
Hi Justin, it is not true that no change in sightly would be required sightly use just tries to adapt resource or request, not any other objects (see ClassUseProvider L65 ff as to be found in the contribution zip). I'm currently searching for the option to solve the case with the least modifications and least impact but this is the constraint that I couldn't eliminate. Cheers Dominik On Fri, Oct 24, 2014 at 4:32 PM, Justin Edelson wrote: > Hi Dominik, > > Anyone is free to define their own objects which implement Adaptable > and their own model classes which can be adapted from those adaptable > classes. This doesn't require any additions to any part of Sling nor > any changes to Sightly. > > Regards, > Justin > > On Fri, Oct 24, 2014 at 10:20 AM, Dominik Süß > wrote: > > Hey everybody, > > > > thinking about this again I also realized that changing the Adaptable > > Interface or having another one creates more issues then solving. In > fact > > it would be way easier to have some generic Wrapperclass that can hold > the > > request and another object and implementing Adaptable. > > > > This would require some change for the Use semanticts of Sightly to wrap > > request and resource (currentResource by default but could be overriden > by > > parameter) and the Models implementation would use this adaptable instead > > of request _or_ resource. > > > > WDYT? > > > > Dominik > > > > On Fri, Oct 24, 2014 at 3:34 PM, Justin Edelson < > jus...@justinedelson.com> > > wrote: > > > >> Hi Stefan, > >> > >> On Fri, Oct 24, 2014 at 7:53 AM, Stefan Seifert > > >> wrote: > >> > > >> >>Personally, I don't quite see the use case -- if your AdapterFactory > >> >>(whether it is generated by Sling Models or hand-coded) needs > >> >>request-based context, the adaptable is the request. > >> > > >> > to shed some more light on my usecase an example: > >> > > >> > - a sightly template calls a "controller" model, which adapts from the > >> current request > >> > - this model wants to output an object structed based on a complex > data > >> structure (e.g. a list of nodes with subnodes) > >> > - this data structure cannot be used directly for output, but some > >> properties need some special processing > >> > - so we build a set of models for this data structures adapting to a > >> resource for each node, injecting each other for @ChildResource for the > >> hierarchy > >> > - each of this resource models does post processing on some > attributes, > >> e.g. > >> > - resolving a media asset reference to a set of externalized image > >> URLs for responsive imaging > >> > - externalizing an URL respecting sling short URL mapping > >> > - injecting dummy images or links for invalid references - but only > if > >> the edit mode of the CMS is active. > >> > - this post processing is done with further models that implement the > >> business logic to solve this centrally > >> > > >> > it is an implementation details of the post processing business models > >> that they need a request for some part of their job (e.g. distinguish > >> between edit and preview mode). the caller of the initial adaption is > not > >> necessary aware of this and should not care of it in my point of view. > >> > >> This is all just an implementation choice. Nothing is forcing you do > >> to the post processing in an object which doesn't have the request. > >> You can just as easily (probably better) do that post processing in an > >> object which has direct access to the request. Or, as Alex suggested, > >> provide the request to those objects once they are adapted. > >> > >> And of course the caller has to be aware that the request is > >> necessary. It is part of the context of that the execution of that > >> post processing. > >> > >> > if you call and OSGi service you do not have to know what services > this > >> has bound internally, OSGi takes care that all are resolved or the > service > >> is not available. > >> > >> I don't think this is the right analogy. You aren't talking about > >> service dependencies, you are talking about the API. If I have a > >> method called getFoo(Object context) and the context object is > >> required for proper execution. If I pass null as the context object, > >> then I shouldn't expect to get a valid response. > >> > >> > > >> > i'm not a fan of the proposals to extend the adaptTo() semantic for > this > >> usecase. not only because its difficult to do it without breaking > existing > >> code (or leads to an interface like Adaptable2), the caller should not > have > >> to now the implementation details for such cross-cutting concern models > >> that are required deeper in the model structure. for the same reason > >> extending the sling models factory interface or having additional > >> "setRequest()" methods on the models are no fitting solutions - the > model > >> that is adapted initially is not the problem, but those further down the > >> hierarchy. > >> > > >> > having an interface like "RequestScope
Re: [RTC] ThreadLocal for getting current request in sling
Hi Dominik, Anyone is free to define their own objects which implement Adaptable and their own model classes which can be adapted from those adaptable classes. This doesn't require any additions to any part of Sling nor any changes to Sightly. Regards, Justin On Fri, Oct 24, 2014 at 10:20 AM, Dominik Süß wrote: > Hey everybody, > > thinking about this again I also realized that changing the Adaptable > Interface or having another one creates more issues then solving. In fact > it would be way easier to have some generic Wrapperclass that can hold the > request and another object and implementing Adaptable. > > This would require some change for the Use semanticts of Sightly to wrap > request and resource (currentResource by default but could be overriden by > parameter) and the Models implementation would use this adaptable instead > of request _or_ resource. > > WDYT? > > Dominik > > On Fri, Oct 24, 2014 at 3:34 PM, Justin Edelson > wrote: > >> Hi Stefan, >> >> On Fri, Oct 24, 2014 at 7:53 AM, Stefan Seifert >> wrote: >> > >> >>Personally, I don't quite see the use case -- if your AdapterFactory >> >>(whether it is generated by Sling Models or hand-coded) needs >> >>request-based context, the adaptable is the request. >> > >> > to shed some more light on my usecase an example: >> > >> > - a sightly template calls a "controller" model, which adapts from the >> current request >> > - this model wants to output an object structed based on a complex data >> structure (e.g. a list of nodes with subnodes) >> > - this data structure cannot be used directly for output, but some >> properties need some special processing >> > - so we build a set of models for this data structures adapting to a >> resource for each node, injecting each other for @ChildResource for the >> hierarchy >> > - each of this resource models does post processing on some attributes, >> e.g. >> > - resolving a media asset reference to a set of externalized image >> URLs for responsive imaging >> > - externalizing an URL respecting sling short URL mapping >> > - injecting dummy images or links for invalid references - but only if >> the edit mode of the CMS is active. >> > - this post processing is done with further models that implement the >> business logic to solve this centrally >> > >> > it is an implementation details of the post processing business models >> that they need a request for some part of their job (e.g. distinguish >> between edit and preview mode). the caller of the initial adaption is not >> necessary aware of this and should not care of it in my point of view. >> >> This is all just an implementation choice. Nothing is forcing you do >> to the post processing in an object which doesn't have the request. >> You can just as easily (probably better) do that post processing in an >> object which has direct access to the request. Or, as Alex suggested, >> provide the request to those objects once they are adapted. >> >> And of course the caller has to be aware that the request is >> necessary. It is part of the context of that the execution of that >> post processing. >> >> > if you call and OSGi service you do not have to know what services this >> has bound internally, OSGi takes care that all are resolved or the service >> is not available. >> >> I don't think this is the right analogy. You aren't talking about >> service dependencies, you are talking about the API. If I have a >> method called getFoo(Object context) and the context object is >> required for proper execution. If I pass null as the context object, >> then I shouldn't expect to get a valid response. >> >> > >> > i'm not a fan of the proposals to extend the adaptTo() semantic for this >> usecase. not only because its difficult to do it without breaking existing >> code (or leads to an interface like Adaptable2), the caller should not have >> to now the implementation details for such cross-cutting concern models >> that are required deeper in the model structure. for the same reason >> extending the sling models factory interface or having additional >> "setRequest()" methods on the models are no fitting solutions - the model >> that is adapted initially is not the problem, but those further down the >> hierarchy. >> > >> > having an interface like "RequestScope" is basicall going into the right >> direction - but who should detect this interface and inject the request? >> the sling models implementation? than we have the same problem not able to >> geht the request objects if it's not the adaptable. and sling models does >> not need such interfaces, its solely based on @Inject annotations. >> >> As I said, this has nothing to do with Sling Models. It is a general >> problem with adapters. >> >> Justin >> >> > >> > this is getting a long thread, i'll write a summary to lead to a >> decision. >> > >> > stefan >> > >>
Re: [RTC] ThreadLocal for getting current request in sling
Hey everybody, thinking about this again I also realized that changing the Adaptable Interface or having another one creates more issues then solving. In fact it would be way easier to have some generic Wrapperclass that can hold the request and another object and implementing Adaptable. This would require some change for the Use semanticts of Sightly to wrap request and resource (currentResource by default but could be overriden by parameter) and the Models implementation would use this adaptable instead of request _or_ resource. WDYT? Dominik On Fri, Oct 24, 2014 at 3:34 PM, Justin Edelson wrote: > Hi Stefan, > > On Fri, Oct 24, 2014 at 7:53 AM, Stefan Seifert > wrote: > > > >>Personally, I don't quite see the use case -- if your AdapterFactory > >>(whether it is generated by Sling Models or hand-coded) needs > >>request-based context, the adaptable is the request. > > > > to shed some more light on my usecase an example: > > > > - a sightly template calls a "controller" model, which adapts from the > current request > > - this model wants to output an object structed based on a complex data > structure (e.g. a list of nodes with subnodes) > > - this data structure cannot be used directly for output, but some > properties need some special processing > > - so we build a set of models for this data structures adapting to a > resource for each node, injecting each other for @ChildResource for the > hierarchy > > - each of this resource models does post processing on some attributes, > e.g. > > - resolving a media asset reference to a set of externalized image > URLs for responsive imaging > > - externalizing an URL respecting sling short URL mapping > > - injecting dummy images or links for invalid references - but only if > the edit mode of the CMS is active. > > - this post processing is done with further models that implement the > business logic to solve this centrally > > > > it is an implementation details of the post processing business models > that they need a request for some part of their job (e.g. distinguish > between edit and preview mode). the caller of the initial adaption is not > necessary aware of this and should not care of it in my point of view. > > This is all just an implementation choice. Nothing is forcing you do > to the post processing in an object which doesn't have the request. > You can just as easily (probably better) do that post processing in an > object which has direct access to the request. Or, as Alex suggested, > provide the request to those objects once they are adapted. > > And of course the caller has to be aware that the request is > necessary. It is part of the context of that the execution of that > post processing. > > > if you call and OSGi service you do not have to know what services this > has bound internally, OSGi takes care that all are resolved or the service > is not available. > > I don't think this is the right analogy. You aren't talking about > service dependencies, you are talking about the API. If I have a > method called getFoo(Object context) and the context object is > required for proper execution. If I pass null as the context object, > then I shouldn't expect to get a valid response. > > > > > i'm not a fan of the proposals to extend the adaptTo() semantic for this > usecase. not only because its difficult to do it without breaking existing > code (or leads to an interface like Adaptable2), the caller should not have > to now the implementation details for such cross-cutting concern models > that are required deeper in the model structure. for the same reason > extending the sling models factory interface or having additional > "setRequest()" methods on the models are no fitting solutions - the model > that is adapted initially is not the problem, but those further down the > hierarchy. > > > > having an interface like "RequestScope" is basicall going into the right > direction - but who should detect this interface and inject the request? > the sling models implementation? than we have the same problem not able to > geht the request objects if it's not the adaptable. and sling models does > not need such interfaces, its solely based on @Inject annotations. > > As I said, this has nothing to do with Sling Models. It is a general > problem with adapters. > > Justin > > > > > this is getting a long thread, i'll write a summary to lead to a > decision. > > > > stefan > > >
Re: [RTC] ThreadLocal for getting current request in sling
Hi Stefan, On Fri, Oct 24, 2014 at 7:53 AM, Stefan Seifert wrote: > >>Personally, I don't quite see the use case -- if your AdapterFactory >>(whether it is generated by Sling Models or hand-coded) needs >>request-based context, the adaptable is the request. > > to shed some more light on my usecase an example: > > - a sightly template calls a "controller" model, which adapts from the > current request > - this model wants to output an object structed based on a complex data > structure (e.g. a list of nodes with subnodes) > - this data structure cannot be used directly for output, but some properties > need some special processing > - so we build a set of models for this data structures adapting to a resource > for each node, injecting each other for @ChildResource for the hierarchy > - each of this resource models does post processing on some attributes, e.g. > - resolving a media asset reference to a set of externalized image URLs for > responsive imaging > - externalizing an URL respecting sling short URL mapping > - injecting dummy images or links for invalid references - but only if the > edit mode of the CMS is active. > - this post processing is done with further models that implement the > business logic to solve this centrally > > it is an implementation details of the post processing business models that > they need a request for some part of their job (e.g. distinguish between edit > and preview mode). the caller of the initial adaption is not necessary aware > of this and should not care of it in my point of view. This is all just an implementation choice. Nothing is forcing you do to the post processing in an object which doesn't have the request. You can just as easily (probably better) do that post processing in an object which has direct access to the request. Or, as Alex suggested, provide the request to those objects once they are adapted. And of course the caller has to be aware that the request is necessary. It is part of the context of that the execution of that post processing. > if you call and OSGi service you do not have to know what services this has > bound internally, OSGi takes care that all are resolved or the service is not > available. I don't think this is the right analogy. You aren't talking about service dependencies, you are talking about the API. If I have a method called getFoo(Object context) and the context object is required for proper execution. If I pass null as the context object, then I shouldn't expect to get a valid response. > > i'm not a fan of the proposals to extend the adaptTo() semantic for this > usecase. not only because its difficult to do it without breaking existing > code (or leads to an interface like Adaptable2), the caller should not have > to now the implementation details for such cross-cutting concern models that > are required deeper in the model structure. for the same reason extending the > sling models factory interface or having additional "setRequest()" methods on > the models are no fitting solutions - the model that is adapted initially is > not the problem, but those further down the hierarchy. > > having an interface like "RequestScope" is basicall going into the right > direction - but who should detect this interface and inject the request? the > sling models implementation? than we have the same problem not able to geht > the request objects if it's not the adaptable. and sling models does not need > such interfaces, its solely based on @Inject annotations. As I said, this has nothing to do with Sling Models. It is a general problem with adapters. Justin > > this is getting a long thread, i'll write a summary to lead to a decision. > > stefan >
Re: [RTC] ThreadLocal for getting current request in sling
Am 24.10.14 um 13:53 schrieb Stefan Seifert: > > i'm not a fan of the proposals to extend the adaptTo() semantic for > this usecase. not only because its difficult to do it without > breaking existing code (or leads to an interface like Adaptable2), > the caller should not have to now the implementation details for such > cross-cutting concern models that are required deeper in the model > structure. +1 this opens a can of worms and especially as the context object would be untyped there is no compile time check to ensure that the correct context object is passed. Carsten -- Carsten Ziegeler Adobe Research Switzerland cziege...@apache.org
RE: [RTC] ThreadLocal for getting current request in sling - summary
a summary of this thread: - there are good ideas how to implement this, if we want to implement it (via the engine, not a servlet filter) - but there are a lot of objections if we should add such a thread-local-based feature at all with good arguments - still there are usecases for some of us (like me) for needing this, and other API changes do not seem to fulfill the goals to proceed I see this alternatives: A) revert my changes from SLING-4083, and do not support this feature in sling models and sling at all B) do not add this feature as a main sling feature with a dedicated API, but only implement as it is currently in SLING-4083 to be usable inside sling models C) add it as main feature to sling as part of the public API. for B) and C) we would need a vote. for A) i still have the possibility to implement this outside sling in a custom sling models injector and a request filter, so i'll be fine with this way as well if the community does not want A) or B). stefan >-Original Message- >From: Stefan Seifert [mailto:sseif...@pro-vision.de] >Sent: Wednesday, October 22, 2014 10:22 AM >To: dev@sling.apache.org >Subject: [RTC] ThreadLocal for getting current request in sling > >i propose to add a new feature to the Sling API and Sling Engine to access to >the current request via an OSGi service, using a servlet filter and a thread >local internally. > >a proposal for such an implementation is currently part of sling models >trunk, but should be renamed and moved to a more central part if we agree if >it is a good idea. interface [1], impl [2]. > >we have already a comparable threadlocal concept for resource resolver [3] > >my current usecases are: >- having the ability to inject context objects like request in any sling >model, regardless of the adaptable. this is e.g. importing when adapting from >a resource, but in context of a request (SLING-4083) >- configuration parameter override provider which injects overrides from HTTP >header for test environments [4] > >stefan > >p.s. as pointed out by justin this topic was discussed already in the past, >e.g. as a side-aspect of this thread [5]. this thread contains some more >usecaes. > > >[1] >https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/s >rc/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorReques >tContext.java > >[2] >https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/s >rc/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorReques >tContextFilter.java > >[3] >http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/Resource >ResolverFactory.html#getThreadResourceResolver-- > >[4] https://github.com/wcm-io/wcm- >io/blob/master/config/core/src/main/java/io/wcm/config/core/override/impl/Req >uestHeaderOverrideProvider.java > >[5] http://sling.markmail.org/thread/epn5vdw3fkmpsk6w
RE: [RTC] ThreadLocal for getting current request in sling
>Personally, I don't quite see the use case -- if your AdapterFactory >(whether it is generated by Sling Models or hand-coded) needs >request-based context, the adaptable is the request. to shed some more light on my usecase an example: - a sightly template calls a "controller" model, which adapts from the current request - this model wants to output an object structed based on a complex data structure (e.g. a list of nodes with subnodes) - this data structure cannot be used directly for output, but some properties need some special processing - so we build a set of models for this data structures adapting to a resource for each node, injecting each other for @ChildResource for the hierarchy - each of this resource models does post processing on some attributes, e.g. - resolving a media asset reference to a set of externalized image URLs for responsive imaging - externalizing an URL respecting sling short URL mapping - injecting dummy images or links for invalid references - but only if the edit mode of the CMS is active. - this post processing is done with further models that implement the business logic to solve this centrally it is an implementation details of the post processing business models that they need a request for some part of their job (e.g. distinguish between edit and preview mode). the caller of the initial adaption is not necessary aware of this and should not care of it in my point of view. if you call and OSGi service you do not have to know what services this has bound internally, OSGi takes care that all are resolved or the service is not available. i'm not a fan of the proposals to extend the adaptTo() semantic for this usecase. not only because its difficult to do it without breaking existing code (or leads to an interface like Adaptable2), the caller should not have to now the implementation details for such cross-cutting concern models that are required deeper in the model structure. for the same reason extending the sling models factory interface or having additional "setRequest()" methods on the models are no fitting solutions - the model that is adapted initially is not the problem, but those further down the hierarchy. having an interface like "RequestScope" is basicall going into the right direction - but who should detect this interface and inject the request? the sling models implementation? than we have the same problem not able to geht the request objects if it's not the adaptable. and sling models does not need such interfaces, its solely based on @Inject annotations. this is getting a long thread, i'll write a summary to lead to a decision. stefan
Re: [RTC] ThreadLocal for getting current request in sling
On 23.10.2014, at 15:39, Alexander Klimetschek wrote: > BTW, this is what I think is great about OSGi services, that they are based > on plain Java interfaces and don't have all the complexities of DI frameworks > such as Sling I mean "Spring" of course :) Alex
Re: [RTC] ThreadLocal for getting current request in sling
On 23.10.2014, at 15:14, Dominik Süß wrote: > > adding some contexobject is IMHO not hidding anything. It is - if you never see that behind the scenes a request object is passed along (and what not else), but in other cases outside the request scope it is not, this can be confusing. It is important to be aware of the request scope - and many folks are easily fooled by that (I know, not you and the others discussing this here). > There are cases > where you would want to have the context for creation of a model (or any > other adaptable) - so at construction time which disqualifieds some method > to be called afterwards. > What't the issue with someresource.adaptTo(MyClass.class, request) (or as > bertrand mentioned any other payload) which could be used by the > AdapterFactory? Maybe, but I'd be reluctant to extend the adaptTo API in this way. (It might be very hard to extend that API, since so many code implements Adaptable). > In case of Scripting languages as sightly we cannot easily create wrapper > objects that are used for adaption that could hold the real adaptable and > the additional context object(s). You model could have a "RequestScoped" or some better named interface with a setRequest() method, which the sightly executor would cast to and use. Just plain Java OO & interfaces... BTW, this is what I think is great about OSGi services, that they are based on plain Java interfaces and don't have all the complexities of DI frameworks such as Sling that try to do everything with magic behind the scenes. Cheers, Alex
Re: [RTC] ThreadLocal for getting current request in sling
Hi Alex,
adding some contexobject is IMHO not hidding anything. There are cases
where you would want to have the context for creation of a model (or any
other adaptable) - so at construction time which disqualifieds some method
to be called afterwards.
What't the issue with someresource.adaptTo(MyClass.class, request) (or as
bertrand mentioned any other payload) which could be used by the
AdapterFactory?
In case of Scripting languages as sightly we cannot easily create wrapper
objects that are used for adaption that could hold the real adaptable and
the additional context object(s). Therefore I would love to have this
option and extend the Sightly use to be able to hand over an additional
object and/or by default trying to add the request object if adapted from a
model. The Sling Model or adaptable could then utilize this request object
or simply ignore it where not necessary.
Best regards
Dominik
On Thu, Oct 23, 2014 at 10:50 PM, Alexander Klimetschek
wrote:
> On 23.10.2014, at 01:04, Dominik Süß wrote:
> >
> > While request.adaptTo(Target.class) would enable an AdapterFactory to get
> > hold of the currentResource there is no chance to get the the custom
> > resource.
>
> Why not have a setter?
>
> MyModel model = resource.adaptTo(MyModel.class)'
> model.setRequest(slingRequest);
>
> or just pass it along with the methods you call, if you only need it in
> some calls:
>
> model.doSomething("foo", slingRequest);
>
>
> IMO it is very important that devs using code are aware that they are
> using the request object and are in a request scope or not, so hiding it is
> not something you want to strive for.
>
> Cheers,
> Alex
>
>
Re: [RTC] ThreadLocal for getting current request in sling
On 23.10.2014, at 01:04, Dominik Süß wrote:
>
> While request.adaptTo(Target.class) would enable an AdapterFactory to get
> hold of the currentResource there is no chance to get the the custom
> resource.
Why not have a setter?
MyModel model = resource.adaptTo(MyModel.class)'
model.setRequest(slingRequest);
or just pass it along with the methods you call, if you only need it in some
calls:
model.doSomething("foo", slingRequest);
IMO it is very important that devs using code are aware that they are using the
request object and are in a request scope or not, so hiding it is not something
you want to strive for.
Cheers,
Alex
Re: [RTC] ThreadLocal for getting current request in sling
Hi, On Thu, Oct 23, 2014 at 10:04 AM, Dominik Süß wrote: > ...To get around that I was thinking if it wouldn't be a good idea to add a > further adaptTo() signature which would allow to add a generic payload > which could contain contextobjects that an adapterFactory could use for > construction.. Reading this thread I was thinking the same - we could add a second "context" parameter to adaptTo(), that's just an Object that the adapter is supposed to understand. -Bertrand
Re: [RTC] ThreadLocal for getting current request in sling
Hi everyone, didn't follow the whole discussion so I might have missed some poin but if I get it right the case where it gets problematic is when I essentially want to adapt to from a resource (that is not the currentResource) without losing the request scope. While request.adaptTo(Target.class) would enable an AdapterFactory to get hold of the currentResource there is no chance to get the the custom resource. To get around that I was thinking if it wouldn't be a good idea to add a further adaptTo() signature which would allow to add a generic payload which could contain contextobjects that an adapterFactory could use for construction.In case of SlingModels everything in this payload could be injected if there is a matching property to inject those. I know its a bold idea but much less magic then ThreadLocals. Cheers Dominik On Thu, Oct 23, 2014 at 8:22 AM, Carsten Ziegeler wrote: > Am 22.10.14 um 23:21 schrieb Alexander Klimetschek: > > On 22.10.2014, at 01:22, Stefan Seifert wrote: > >> i propose to add a new feature to the Sling API and Sling Engine to > access to the current request via an OSGi service, using a servlet filter > and a thread local internally. > > > > -1 > > > >> a proposal for such an implementation is currently part of sling models > trunk, but should be renamed and moved to a more central part if we agree > if it is a good idea. interface [1], impl [2]. > > > > Why? This looks like a hack to me, and will lead to people not designing > their APIs properly (e.g. passing the request through). I see ThreadLocal > as a workaround when you don't have influence on the API but still need to > pass things around. But in this case (sling models and the wcm.io config, > below) there is full control over the API. > > > > A class like SlingObjectInjectorRequestContext sound very much like > anti-patterns from Spring et al. > > > >> we have already a comparable threadlocal concept for resource resolver > [3] > > > > Broken window :D > > Well, it's easy to judge on a high level without knowing the details. > We're all aware of the dangers of thread locals, but matter of fact is, > there are use cases where you need the resource resolver and there are > tons of layers in between the code using the resource resolver and where > it is available. In some cases the api is not even created in Sling, so > there is absolutely no way to pass the resolver down to the code other > than using a thread local. > > And instead of creating an island solution for every use case, so we end > up with dozens of thread locals and filters and whatnot, having a > general single solution is imho preferable. > > However, I see/have use cases for the resource resolver, I'm not 100% > about the request yet :) > > Regards > Carsten > -- > Carsten Ziegeler > Adobe Research Switzerland > cziege...@apache.org >
Re: [RTC] ThreadLocal for getting current request in sling
Am 22.10.14 um 23:21 schrieb Alexander Klimetschek: > On 22.10.2014, at 01:22, Stefan Seifert wrote: >> i propose to add a new feature to the Sling API and Sling Engine to access >> to the current request via an OSGi service, using a servlet filter and a >> thread local internally. > > -1 > >> a proposal for such an implementation is currently part of sling models >> trunk, but should be renamed and moved to a more central part if we agree if >> it is a good idea. interface [1], impl [2]. > > Why? This looks like a hack to me, and will lead to people not designing > their APIs properly (e.g. passing the request through). I see ThreadLocal as > a workaround when you don't have influence on the API but still need to pass > things around. But in this case (sling models and the wcm.io config, below) > there is full control over the API. > > A class like SlingObjectInjectorRequestContext sound very much like > anti-patterns from Spring et al. > >> we have already a comparable threadlocal concept for resource resolver [3] > > Broken window :D Well, it's easy to judge on a high level without knowing the details. We're all aware of the dangers of thread locals, but matter of fact is, there are use cases where you need the resource resolver and there are tons of layers in between the code using the resource resolver and where it is available. In some cases the api is not even created in Sling, so there is absolutely no way to pass the resolver down to the code other than using a thread local. And instead of creating an island solution for every use case, so we end up with dozens of thread locals and filters and whatnot, having a general single solution is imho preferable. However, I see/have use cases for the resource resolver, I'm not 100% about the request yet :) Regards Carsten -- Carsten Ziegeler Adobe Research Switzerland cziege...@apache.org
Re: [RTC] ThreadLocal for getting current request in sling
On Wed, Oct 22, 2014 at 5:44 PM, Alexander Klimetschek
wrote:
> On 22.10.2014, at 14:36, Justin Edelson wrote:
>> To be clear, Sling Models was designed *not* to have an explicit API
>> and to be a way to implement of AdapterFactories. For version 1.2.0,
>> it does have an API, but this is essentially just a mirror of the
>> AdapterManager API plus some utility methods.
>
> On 22.10.2014, at 14:34, Stefan Seifert wrote:
>> sling models is based on a very simple API: adaptTo().
>>
>> adaptTo passes in only the adaptable, and if this is not the request object
>> there is no chance for the sling models implementation to get hold of it
>> besides some "not-so-clean" way using a threadlocal.
>
> Maybe that's the problem and sling models should have a better way to be
> created instead of piggy-backing on the magic adaptTo().
Except what Stefan is highlighting is actually a generalized problem
with the adapter subsystem and not at all specific to Sling Models.
His use case could just as easily be rephased as "I want to adapt an
instance of Foo and in my AdapterFactory get to Bar (which is not
navigable from Foo), but I don't want the caller to have to worry
about this.", the outcome would be the same.
The only distinction is that these particular AdapterFactory
implementations are generated by Sling Models. This, however, is a
distinction without difference.
Personally, I don't quite see the use case -- if your AdapterFactory
(whether it is generated by Sling Models or hand-coded) needs
request-based context, the adaptable is the request.
>
> For JSPs: a taglib. For Java services: a service with methods like
> getModel(type, request) and getModel(type, resolver) etc. Imagine there is no
> ThreadLocal and adaptTo() available, how would you solve it in the most
> convenient fashion?
Such an API now exists, but I don't see how it changes this
discussion. The only way I see this API helping the use case is to
pass some kind of context object into the getModel(), e.g.
getModel(type, resource, Collections.singletonMap("request",
request)). Is this along the lines of what you are suggesting? And
again, if the caller knows they need to pass in the request, then the
adaptable should be the request.
Regards,
Justin
>
> Cheers,
> Alex
Re: [RTC] ThreadLocal for getting current request in sling
On 22.10.2014, at 14:36, Justin Edelson wrote: > To be clear, Sling Models was designed *not* to have an explicit API > and to be a way to implement of AdapterFactories. For version 1.2.0, > it does have an API, but this is essentially just a mirror of the > AdapterManager API plus some utility methods. On 22.10.2014, at 14:34, Stefan Seifert wrote: > sling models is based on a very simple API: adaptTo(). > > adaptTo passes in only the adaptable, and if this is not the request object > there is no chance for the sling models implementation to get hold of it > besides some "not-so-clean" way using a threadlocal. Maybe that's the problem and sling models should have a better way to be created instead of piggy-backing on the magic adaptTo(). For JSPs: a taglib. For Java services: a service with methods like getModel(type, request) and getModel(type, resolver) etc. Imagine there is no ThreadLocal and adaptTo() available, how would you solve it in the most convenient fashion? Cheers, Alex
Re: [RTC] ThreadLocal for getting current request in sling
Hi, On Wed, Oct 22, 2014 at 5:21 PM, Alexander Klimetschek wrote: > On 22.10.2014, at 01:22, Stefan Seifert wrote: >> my current usecases are: >> - having the ability to inject context objects like request in any sling >> model, regardless of the adaptable. this is e.g. importing when adapting >> from a resource, but in context of a request (SLING-4083) > > That should be possible to solve differently, such injection sounds like too > much magic to me. Why can't sling models have a clear API that passes in such > objects when they are created instead of going through a threadlocal? To be clear, Sling Models was designed *not* to have an explicit API and to be a way to implement of AdapterFactories. For version 1.2.0, it does have an API, but this is essentially just a mirror of the AdapterManager API plus some utility methods. "Such objects" are passed in currently, assuming they are navigable from the adaptable and this is documented in http://sling.staging.apache.org/documentation/bundles/models.html#available-injectors. But Stefan is describing a use case where those objects are not navigable. Justin
RE: [RTC] ThreadLocal for getting current request in sling
>> my current usecases are: >> - having the ability to inject context objects like request in any sling >model, regardless of the adaptable. this is e.g. importing when adapting from >a resource, but in context of a request (SLING-4083) > >That should be possible to solve differently, such injection sounds like too >much magic to me. Why can't sling models have a clear API that passes in such >objects when they are created instead of going through a threadlocal? sling models is based on a very simple API: adaptTo(). adaptTo passes in only the adaptable, and if this is not the request object there is no chance for the sling models implementation to get hold of it besides some "not-so-clean" way using a threadlocal. but it's not always possible nor desired to just use the request as adaptable. e.g. if you handle multiple different resources inside your request and adapt them to a model and need access to the request there is no other way i can think of. if fully agree that this way to access a request should not be the "standard way", and in most usecases there should be a better way (although not in the wcm.io configuration example because it as well relies on adaptTo(), but do not discuss this here in this thread further). SlingObjectInjectorRequestContext was only an name internal to the implementation of sling models. stefan
Re: [RTC] ThreadLocal for getting current request in sling
On 22.10.2014, at 01:22, Stefan Seifert wrote: > i propose to add a new feature to the Sling API and Sling Engine to access to > the current request via an OSGi service, using a servlet filter and a thread > local internally. -1 > a proposal for such an implementation is currently part of sling models > trunk, but should be renamed and moved to a more central part if we agree if > it is a good idea. interface [1], impl [2]. Why? This looks like a hack to me, and will lead to people not designing their APIs properly (e.g. passing the request through). I see ThreadLocal as a workaround when you don't have influence on the API but still need to pass things around. But in this case (sling models and the wcm.io config, below) there is full control over the API. A class like SlingObjectInjectorRequestContext sound very much like anti-patterns from Spring et al. > we have already a comparable threadlocal concept for resource resolver [3] Broken window :D > my current usecases are: > - having the ability to inject context objects like request in any sling > model, regardless of the adaptable. this is e.g. importing when adapting from > a resource, but in context of a request (SLING-4083) That should be possible to solve differently, such injection sounds like too much magic to me. Why can't sling models have a clear API that passes in such objects when they are created instead of going through a threadlocal? > - configuration parameter override provider which injects overrides from HTTP > header for test environments [4] This shows that wcm.io is doing too much magic... also, what do you mean by test environments? That seems very specific, not sure if that validates such a change in the main sling api. > [5] http://sling.markmail.org/thread/epn5vdw3fkmpsk6w Those use cases fall under the "have to use ThreadLocal because we can't change the API", so IMO don't warrant to put that into Sling API, it should be hidden in those modules that need to do the ThreadLocal workaround for their integration. Citing Bertrand from that thread: "Passing the request to a method clearly indicates that you expect that method to deal with it. It's a few more characters to type, but it makes things clear. IMHO, it's too easy to create a mess with threadlocals, which are just another kind of global variables." Cheers, Alex
Re: [RTC] ThreadLocal for getting current request in sling
Am 22.10.14 um 10:52 schrieb Bertrand Delacretaz:
> Hi,
>
> On Wed, Oct 22, 2014 at 10:22 AM, Stefan Seifert
> wrote:
>> ...i propose to add a new feature to the Sling API and Sling Engine to
>> access to the current request via an OSGi service, using
>> a servlet filter and a thread local internally
>
> +1 in principle, though as Carsten says we can implement this in the
> engine to avoid the filter.
>
>> ...we have already a comparable threadlocal concept for resource resolver
>> [3]...
>
> So maybe we should rather put a single"request context" object in a
> ThreadLocal, instead of having different objects manage their own
> thread local things?
>
> Something like
>
> public interface ThreadLocalContext {
> SlingHttpServletRequest getCurrentRequest();
> ResourceResolver getCurrentResourceResolver();
> ResourceResolver [] getActiveResourceResolvers();
> }
>
The resource resolver thread local works outside of a request, so I dont
think we should deprecate that and replace it with the above mechanism.
Let's keep things separate.
Carsten
> etc...we can then test and evolve this in a cleaner way than with code
> scattered around various classes.
>
> For backwards compatibility, we can deprecate [3] and let it use this
> new API for now.
>
> -Bertrand
>
> [3]
> http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver--
>
--
Carsten Ziegeler
Adobe Research Switzerland
cziege...@apache.org
Re: [RTC] ThreadLocal for getting current request in sling
Hi,
On Wed, Oct 22, 2014 at 10:22 AM, Stefan Seifert wrote:
> ...i propose to add a new feature to the Sling API and Sling Engine to access
> to the current request via an OSGi service, using
> a servlet filter and a thread local internally
+1 in principle, though as Carsten says we can implement this in the
engine to avoid the filter.
> ...we have already a comparable threadlocal concept for resource resolver
> [3]...
So maybe we should rather put a single"request context" object in a
ThreadLocal, instead of having different objects manage their own
thread local things?
Something like
public interface ThreadLocalContext {
SlingHttpServletRequest getCurrentRequest();
ResourceResolver getCurrentResourceResolver();
ResourceResolver [] getActiveResourceResolvers();
}
etc...we can then test and evolve this in a cleaner way than with code
scattered around various classes.
For backwards compatibility, we can deprecate [3] and let it use this
new API for now.
-Bertrand
[3]
http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver--
Re: [RTC] ThreadLocal for getting current request in sling
Am 22.10.14 um 10:22 schrieb Stefan Seifert: > i propose to add a new feature to the Sling API and Sling Engine to access to > the current request via an OSGi service, using a servlet filter and a thread > local internally. I think *if* we're doing this, we should add this into the engine and avoid the filter completely. The request object changes for every include and the engine is handling this already, so adding the code there should be simple. Carsten > > a proposal for such an implementation is currently part of sling models > trunk, but should be renamed and moved to a more central part if we agree if > it is a good idea. interface [1], impl [2]. > > we have already a comparable threadlocal concept for resource resolver [3] > > my current usecases are: > - having the ability to inject context objects like request in any sling > model, regardless of the adaptable. this is e.g. importing when adapting from > a resource, but in context of a request (SLING-4083) > - configuration parameter override provider which injects overrides from HTTP > header for test environments [4] > > stefan > > p.s. as pointed out by justin this topic was discussed already in the past, > e.g. as a side-aspect of this thread [5]. this thread contains some more > usecaes. > > > [1] > https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorRequestContext.java > > [2] > https://svn.apache.org/repos/asf/sling/trunk/bundles/extensions/models/impl/src/main/java/org/apache/sling/models/impl/injectors/SlingObjectInjectorRequestContextFilter.java > > [3] > http://sling.apache.org/apidocs/sling7/org/apache/sling/api/resource/ResourceResolverFactory.html#getThreadResourceResolver-- > > [4] > https://github.com/wcm-io/wcm-io/blob/master/config/core/src/main/java/io/wcm/config/core/override/impl/RequestHeaderOverrideProvider.java > > [5] http://sling.markmail.org/thread/epn5vdw3fkmpsk6w > > -- Carsten Ziegeler Adobe Research Switzerland cziege...@apache.org
