Hi Carsten,
now, I totally agree that we should get rid of the rewriter pipeline.
With HTL we can do the rewriting links without reparsing the html
output.
perfect, for me that's one of the main goals of the new SPI. After
the hackathon last year there were some voices arguing pro rewriter
pipeline with valid use cases, but I think those are relevant for
only 5% of the projects (those can still use the pipeline, but
95% should be able to do without). Btw. the link checker use case
(that potentially removes <a href> tags) will bring a little bit
of complexity, but the plan is to allow "Resource URI context hints"
as result of a the mapping chain (and a hint "invalid uri" will make
HTL remove the surrounding tag)
We should have an API that can be used for this, whether this is
rr.map() or something new is a different question. I have no problem
with having rr.map() doing this.
I'm in favour of not introducing a new API if it's not absolutely
needed. But calling out to another service from rr.map()/rr.resolve()
should work fine.
Today mapping (and therefore reverse mapping) is not based on the
current user context - and I think we should keep it this way. Meaning
for any user mapping and reverse mapping creates the same result
regardless of the user. Following this logic, it doesn't make sense to
have this functionality in RR - as the RR is bound to a user. So I
suggest to make this a separate service which can be invoked without
having a RR. We can change the implementation of rr.map() to call that
service. rr.resolve can be changed to first call that service, too,
and then resolve the resulting path.
Ok fair enough - so your point was that the both map() and resolve()
do not (or should not) produce different results based on the user
context, my point was that to be future proof in the SPI, we need
"access to the repository" to address all use cases. So "that service"
in the current implementation would be ResourceUriMappingChain [1],
it currently has access to a rr via
MappingChainContext.getResourceResolver() [2] - I think a resolver
needs to be available to do lookups in the repository, but this
could also be one based on a service user (ready only rights on
the whole repo suffice). So to start with it it could work fine to
hook in ResourceUriMappingChain.resolve() in the request processing
before the resource resolver is created. Maybe then in the future
we could provide a service property to ResourceToUriMapping providers
that allows to be called with the request's user context after the
resource resolver is created (so moving the default mapping chain
before the authentication at least does not block the possibility
to allow user based resolutions for the future). Also the user
context could be generally be made available to rr.map(),
I suppose there it wouldn't hurt (and it would allow to add users
specific information to certain URLs, I think this can be quite
useful). But to start with, would it be ok for you to leave
MappingChainContext.getResourceResolver() [2] as is and provide
a service user rr for repository access to the resolve operation?
-Georg
[1]
https://github.com/apache/sling-org-apache-sling-resourceresolver/blob/feature/SLING-9662-Introduce-Resource-Mapping-SPI/src/main/java/org/apache/sling/resourceresolver/impl/mappingchain/ResourceUriMappingChain.java
[2]
https://github.com/apache/sling-org-apache-sling-api/blob/3eedd71ec46e75ec93848282f7d3cb4a7ce862b5/src/main/java/org/apache/sling/api/resource/mapping/spi/MappingChainContext.java#L32
Does that make sense?
Regards
Carsten
Am 24.08.2020 um 23:58 schrieb Georg Henzler:
Hi Carsten,
And if you still need this, you can do this differently without using
the resource mapping functionality.
This happens too often today because of the missing extensibility of
the resource resolver. The point of the Resource Mapping SPI is to
improve the situation and to be able to fully rely on rr.map()
That way we can make default mapping happen in HTL and get rid
of the rewriter pipeline for 95% of the projects IMHO, if rr.map()
only does "half the job" this cannot easily be achieved
...but that comes with the penalty
of doing these things twice - first in auth core and then again with
the user context - and hopefully both operations provide the same
result (another reason why per user context mapping is dangerous).
The idea was to check the authentication requirement only once in
auth core (for the alias/vanity path case the resolve method will
have to make use of the same map, but I'd expect it to not be too
much of a problem)
Mapping is different from resolving and I think we should start
treating it differently.
IMHO both yes and no:
Yes: Mapping happens at a completely different time than resolving,
mapping creates links to be resolved in the future (even if you
provide the current request to map(), it'll act as example for a
future request.
No: map/resolve "amendments" to the setup should always be made
symmetrically - ResourceToUriMapper with both map() and resolve()
ensures exactly that by forcing SPI providers to think about both
aspects.
Now for SLING-9622 it's actually the rr.resolve() path that is
causing the headache (and "unauthenticated map()" would not help
here). If you like I can give it a try with an SPI interface
ResourceUriAuthRequirementChecker...
-Georg
On 2020-08-24 18:32, Carsten Ziegeler wrote:
Hi Georg,
at least today, the user context is not really taken into account for
mapping. And I think everything stays nicer and cleaner if we don't
open this up - otherwise you need to think about setting correct
caching headers etc.
And if you still need this, you can do this differently without using
the resource mapping functionality.
The primary concern is that authentication handlers and requirements
can be handled correctly and without much effort. Of course we can
add
more methods to the SPI and potentially other services to allow the
auth core to do the correct lookups, but that comes with the penalty
of doing these things twice - first in auth core and then again with
the user context - and hopefully both operations provide the same
result (another reason why per user context mapping is dangerous).
Mapping is different from resolving and I think we should start
treating it differently. If we can agree that mapping is independent
of the user context (and today that is the case), then we can
simplify
things and make them cleaner.
Regards
Carsten
Am 24.08.2020 um 17:09 schrieb Georg Henzler:
Hi Carsten,
good opportunity to rethink mapping / resolving in general.
so you are referring to SLING-9622 and more specifically to comment
[1] I suppose... I have thought it a bit through and I think there
are valid use cases for having "the user context" (=rr) available
for both resolve() and map(). But if it's really just about a
knowing early (in processor) if the incoming request requires
authentication or not, why not just add a method
requiresAuthentication(resourceUri) to SPI at [2]? Or maybe cleaner,
a second interface ResourceUriAuthenticationRequirementChecker could
be introduced (that "interested" ResourceToUriMapper could also
implement). Once vanity path and alias handling are migrated to this
new setup, the SPI method can access the readily available data
structure, so this should not have a performance impact... WDYT?
-Georg
[1]
https://issues.apache.org/jira/browse/SLING-9622?focusedCommentId=17182336&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17182336
[2]
https://github.com/apache/sling-org-apache-sling-api/blob/3eedd71ec46e75ec93848282f7d3cb4a7ce862b5/src/main/java/org/apache/sling/api/resource/mapping/spi/ResourceToUriMapper.java#L35
On 2020-08-23 11:24, Carsten Ziegeler wrote:
Thanks for taking this up.
As noted in the issue, I think we should investigate whether this
is a
good opportunity to rethink mapping / resolving in general. In
hindsight, we probably shouldn't have added the mapping part to the
resource resolver. The resolve() method does two things: it applies
mappings (which is independent of the current user) and it resolves
to
a resource (finding the resource and figuring out the selectors,
extensions etc.) based on the user.
So how about moving the mapping part out of the resource resolver?
As
we've seen recently other parts of Sling like auth core could
benefit
from this as well.
I haven't fully thought this through, but this is the rough idea:
We create a new OSGi service, PathMapperFactory (its probably not a
good name but I don't want to invest time into naming at this
stage).
It has one method:
PathMapper newPathMapper(HttpServletRequest request)
where the request parameter is optional. PathMapper has the
resolve/map methods doing the mapping (based on the newly suggested
SPI). If a request has been provided to the factory method, that
object is used for resolving/mapping.
I'm unsure about the exact methods of PathMapper, but it needs a
resolve method taking in a path (the request path) and returning
the
resolved path (vanity path, aliases etc are applied) and a flag
whether a redirect should happen.
Sling's main servlet (or more precise the processor) is changed:
for
an incoming request it calls the PathMapperFactory with the request
and then the resolve method on PathMapper. If the result is absolut
or
the redirect flag is set, the redirect happens.
Otherwise, authentication is invoked with the resolved path - this
frees auth core (and all auth handlers etc) from dealing with
mappings.
If authentication is successful, a new resource resolver is created
and the PathMapper is passed to the resource resolver via the
attributes of the resource resolver factory. ResourceResolver gets
a
new method "getPathMapper()" - if no PathMapper is passed to the
resource resolver factory, the factory creates a new one by
invoking
newPathMapper(null) on the PathMapperFactory. In all cases, a
resource
resolver has a PathMapper.
We add a new method to Resource Resolver, similar to the resolve()
method but just doing the resolution part - no mapping. This method
is
used by the main servlet instead of the existing resolve() method.
We
deprecate resolve() and map() and change the implementation to
internally use the passed in PathMapper instance.
This way we have a clear separation between mapping and resolving
and
in which contexts the operations happen. With adding the PathMapper
to
the resource resolver we make it available to all parts in the
system
which need to call todays map() method - and as the PathMapper
instance knows the request, there is no need to pass it around
anymore.
Now, again, just a rough write down of the idea, don't get hung up
on
the naming that can and needs to improve. Its also not directly
about
the new SPI, but rather how we make the SPI available to the rest
of
the system.
Regards
Carsten
