symfony-cmf/routing-auto

Conflict resolving not well handled when generating conflicting routes within the same URI Context Collection

damienflament opened this issue · 15 comments

damienflament commented

The initial discussion about this issue can be found in the Routing Auto Bundle repository symfony-cmf/routing-auto-bundle#201.

The issue

Actually, the AutoRouteManager handles conflicting routes by searching for existing routes which match the same URI as the route being created. But this search is done only within the already persisted routes using the adapter.

The point is that the AutoRouteManager does not persist routes after their creation. It is done by the AutoRouteListener and the RefreshCommand. So, if two routes match the same URI within the URI Context Collection being built, the AutoRouteManager does not take care of that. The issue is actually revealed when the routes are persisted by the Routing Auto Bundle.

I tried to fix this issue, but a second problem appears: the AutoRouteManager can search for existing routes within the URI Context Colection being built, but the Conflict Resolver is not aware of the whole URI Context Collection, just the current URI Context. That means it cannot fix the conflict.

damienflament commented

Just a clarification : I think about a solution to fix this issue, but it will break the API:

Another solution which seems to be the best one, is to consider the Adapter to be in charge of the persisting operations (which seems obvious). The Adapter implementation within the Routing Auto Bundle follows this principle for some operations (migrateAutoRouteChildren, removeAutoRoute) but not for the creation of routes (createAutoRoute, createRedirectRoute).

dantleech commented

Hey, thanks for the detailed issue.

I suppose my first question is do your documents have the same URLs? Why not /fr/sculptures and /en/sculptures? The conflict resolver is a last-resort - why should english users see /sculptures-1 and french users /sculptures? (if I understand correctly)

But I agree the system should be clever enough to deal with this.

make the AutoRouteManager check for existing routes....

I think this is my preferred solution. The AutoRouteManager can take responsiblity for this, we don't need to bother the persistence adapter about it.

The adapter might need to have an explicit operation to persist the route, but as the only official adapter is the PHPCR-ODM one, and it is not-trivial, this has been ommited as the responsiblity will be clearer when developing, f.e. the Doctrine ORM adapter.

damienflament commented

I suppose my first question is do your documents have the same URLs? Why not /fr/sculptures and /en/sculptures

The only reason I don't inject the locale in the URI is to have shorter/smarter URLs. I know it's not a common practice, but I'm experimenting Symfony and especially PHPCR-ODM/Symfony CMF to build a simple CMS, the more user-friendly I can do, using ContentTools as an inline-editing UI.

The simplest solution to my issue is to use the locale in the URL. But I added this issue ticket because it is related to a more generic case : a conflict between (at least) two URL within the currently built URIContextCollection.

The adapter might need to have an explicit operation to persist the route

The adapter not necessarily needs an explicit persist operation, as the implementation of the PHPCR-ODM Adapter already persists some modifications (as mentioned in my previous post). The only addition might me to make explicit in the AdapterInterface documentation the need to persist the modification.

Here is a naive implementation of the two solution I think about :

Make the AutoRouteManager search within the current URIContextCollection

public function buildUriContextCollection(UriContextCollection $uriContextCollection)
{
    // ...
    $uri = $this->uriGenerator->generateUri($uriContext);
    $uriContext->setUri($uri);
    $existingRoute = $this->findExistingRoute($uri, $uriContextCollection);
    // ...
}

private function findExistingRoute($uri, UriContextCollection $uriContextCollection)
{
    foreach ($uriContextCollection->getUriContexts() as $uriContext) {
        if (! is_null($$uriContext->getAutoRoute()) && $uriContext->getUri() === $uri) {
            return $autoRoute;
        }
    }

    return $this->adapter->findRouteForUri($uri, $uriContext);
}

But the ConflictResolver needs to be aware of the current URIContextCollection too. And it is not actually the case :

public function resolveConflict(UriContext $uriContext);

So, an API change is required.

Make the Adapter persist the routes

Routing Auto Bundle PHPCR-ODM Adapter:

public function createAutoRoute(UriContext $uriContext, $contentDocument, $locale)
{
    // ...

    // The intermediate document are already persisted.
    foreach ($segments as $segment) {
        $basePath .= '/'.$segment;
        $document = $this->dm->find(null, $basePath);
        if (null === $document) {
            $document = new Generic();
            $document->setParentDocument($parentDocument);
            $document->setNodeName($segment);
            $this->dm->persist($document);
        }
        $parentDocument = $document;
    }

    // ...

    // return $headRoute;

    // Instead of returning the route, persist it.
    $this->dm->persist($headRoute);
}

Routing Auto Bundle AutoRouteListener :

public function onFlush(ManagerEventArgs $args)
{
    // ...

    $uriContextCollection = new UriContextCollection($document);
    $arm->buildUriContextCollection($uriContextCollection);

    // This is not needed anymore.
    //
    // foreach ($uriContextCollection->getUriContexts() as $uriContext) {
    //     $autoRoute = $uriContext->getAutoRoute();
    //     $dm->persist($autoRoute);
    //     $uow->computeChangeSets();
    // }

    // ...
}

This solution don't need any API change. Moreover, some documents are already persisted.

damienflament commented

I can do a PR including additional tests to reveal this issue and fix it. I just need to make a development environment for the Routing Auto Bundle/Component.

But I wanted to show you my analysis as you (the maintainers of Symfony CMF) know better than me its mechanics.

damienflament commented

Anything new about that ? Any advice ?

dantleech commented

Will try and have a closer look tonight

dantleech commented

Make the Adapter persist the routes

I think the issue there is that if you have a Doctrine ORM adapter, then the URL will be a field in an entity, and you would need to flush the EM before the findByUrl method would work.

Also cannot remember if there are any other reasons for the current behavior.

Make the AutoRouteManager search within the current URIContextCollection

Yes, I think handling this in the component would ultimately be better.

But I am not sure that this is worth the effort - do you think it is a good idea to have /article in French and /article-1 in English? I think internationalised URLs should be "namespaced" (i.e. prefix on URL (/fr/article) or different subdomain (fr.mydomain.com/article) etc), I believe that is a best practice and most "user friendly".

I do agree that the system should handle this case - but the case should not be encouraged in my opinion.

Fixing this issue would probably require a BC break.

damienflament commented

But I am not sure that this is worth the effort - do you think it is a good idea to have /article in French and /article-1 in English?

In fact, the URL will not be /article. It will be /realisations/vasques-en-cuivre-polies in french and /works/polished-copper-sinks. So, URL conflict for articles will never happen. But editable pages have not any prefix (to make the URL smarter). And actually, the page which reveals the issue is titled Structures (the same word is used in both languages).

I agree that including the locale in the URL is the common way, but I think it's not more "user friendly" as most visitors don't see the website in multiple languages. It's just the easiest way to avoid issues for the developer.

[...] but the case should not be encouraged in my opinion

I think that tools don't have to restrict the use cases. But actually, it is not clearly mentioned in the documentation, it's restricted by some weird error messages which, after some analysis, reveal a bug in the Routing Auto Bundle (which try to persist a route with a existing nodename/path).

I post this issue on the component repository because I think the bug is, in fact, due to an issue with the AutoRouteManager. It's not related to the locale. It's related to 2 URIContexts conflicting within the same URIContextCollection. There is a lot of possibilities to generate 2 conflicting contexts as a URIContextCollection does not contains the routes for the different locales of the same document. It contains the routes for the different definitions.

And definitions can conflicts on many ways, as they can be defined by schemes, methods, token providers.

damienflament commented

I understand that you (the Symfony CMF developers) may not want to handle this issue due to the low number of affected developers (actually, just me I think).

If this tools can't feet my needs, I can do it without this component. Actually, I'm experimenting with the core Symfony routing system. And using a custom route loader seems to work just fine.

dbu commented

regardless of the discussion whether the situation with translations is a good use case or not, i think this is a bug that should be fixed. i can think of any number of situations where auto urls collide (like slugifying titles and somebody writing an article with a title that already exists or whatever).

i am not familiar enough with the code but would think that the component that is building routes could keep track of what it created and solve collisions on that without even needing the collision checker.

damienflament commented

i think this is a bug that should be fixed

OK. I can submit a PR. But I want to be sure about the chosen solution as it implies working on the component or the bundle or both.

i am not familiar enough with the code but would think that the component that is building routes could keep track of what it created and solve collisions on that without even needing the collision checker.

The "collision checker" is just a call to the findRouteForUri() method on the Adapter.

That's why I think a clarification in the AdapterInterface documentation stating that the adapter needs to persist (maybe flushing also) the modifications is the best solution:

Moreover, the needed modifications are minors:

  • make clear that the created object needs to be persisted (maybe flushed ?) within the component AdapterInterface related methods,
  • slightly modify the bundle PhpcrOdmAdapter implementation to persist the created document before returning it.
  • remove the no more needed persistence operations within the AutoRouteListener (used when "auto routable documents" are flushed) and the RefreshCommand (used manually through the Symfony console) of the bundle.
dantleech commented

I would prefer an in-band solution, the prospective conflicting routes here can be resolved without reference to the storage?

It is not desirable to commit a transaction for each translation[1] (if that is what you are suggesting). In anycase I am not sure that persisting will suffice to enable the location of routes using the adapters method - it's not the storage layers responsibilty.

My view without looking too deeply then is that we should resolve conflicts within the UriContextCollection in co-operation with checking for existing routes in the storage (in a recursive fashion).

But without diving into this I don't know, so please feel free to play around :)

[1] PHPCR-ODM forces our hand here because of it's fixed order of Operations.

damienflament commented

It is not desirable to commit a transaction for each translation

Right ! I wondered if the DocumentManager::find() method searches in the persisted but not flushed data (within the unit of work, I think). But the real question is: Can any persistence implementation search within not committed data ? I think we can't rely on the actual implementation of the Adapter.

My view without looking too deeply then is that we should resolve conflicts within the UriContextCollection

Resolving conflicts within the UriContextCollection seems easy as we can rely on the value of the URI of each URIContext (which is null for the not processed contexts). So, we don't need to maintain an additional list of the generated URIs.

The only problematic needed modification is within the conflict resolver. It needs to take care of the currently processed UriContextCollection to resolve the conflict as it actually validates the new generated URI using the adapter.

in co-operation with checking for existing routes in the storage (in a recursive fashion).

Where do you see a recursive operation ? I didn't think about any recursivity here.

dantleech commented
dantleech commented
  • 👍1