pradosoft / prado

Prado - Component Framework for PHP
Other
186 stars 70 forks source link

RFC: Service detection is fragile #868

Closed ctrlaltca closed 1 year ago

ctrlaltca commented 1 year ago

An PRADO application can expose multiple services; usually there's a single "page "service, but more can be defined in application.xml. The way PRADO detects which service to run is implemented in THttpRequest: https://github.com/pradosoft/prado/blob/master/framework/Web/THttpRequest.php#L761 This looks a bit fragile.

I have an example application containing 2 services: a "page" service hosting a website and a "geonode" service, accessed by desktop applications like QGIS. Requests to the "page" service runs fine with urls like http://127.0.0.1/prado-app/page,Home. Requests to the "geonode" service are a bit more complex and contains parameters hardcoded in the client, eg: http://127.0.0.1/prado-app/geonode/api/v2/layers/?page=1&page_size=10&sort[]=name&filter%7BstoreType.in%7D=dataStore&filter%7BstoreType.in%7D=coverageStore A problem arises on the second request: Prado merges all the request parameters in a single array, and loops through the array to look for a service id. The "page=1" parameter gets interpreted as a request to the "page" service, and since it's the first service matching in the array it gets used, even if it's not the correct one. Now, since the request parameters are hardcoded in the client (https://github.com/GeoNode/QGISGeoNodePlugin/blob/main/src/qgis_geonode/apiclient/geonode_v3.py#L56), i can't really fix them but i need a way to workaround the problem, forcing PRADO to use the correct service.

I'll try to workaound this in my app overloading the THttpRequest::resolveRequest() method, hopefully finding a way extract all matching services and giving priority to the one appearing first. If you have any comment on other ways to fix this, i'd be glad to hear from you :)

ctrlaltca commented 1 year ago

This is the first working attempt:

    public function resolveRequest($serviceIDs)
    {
        Prado::trace("Resolving request from " . $_SERVER['REMOTE_ADDR'], 'Prado\Web\THttpRequest');
        $getParams = $this->parseUrl();
        // Prepend parameters extracted from the URL so they gets first in the parameters array
        $this->_items = array_merge($getParams, $_GET, $_POST);
        $this->_requestResolved = true;
        /*
         * Instad of looping all services in the application.xml order,
         * loop al parameters in the order received in the request.
         */
        foreach ($this->_items as $serviceID => $value)
        {
            if(in_array($serviceID, $serviceIDs))
            {
                $this->setServiceID($serviceID);
                $this->setServiceParameter($this->itemAt($serviceID));
                return $serviceID;
            }
        }
        return null;
    }

The changes are commented; the main change is that while matching service IDS with request parameters now it uses the order of parameters received in the request instead of the order of services defined in application.xml .

drigolin commented 1 year ago

Why don't use the "path url prefix" as a main selector for a service? /prado-app/page/pageName. / prado-app/geonode/

Can be optional in the service parameter to avoid breaking changes.

ctrlaltca commented 1 year ago

I thought about implementing a different TUrlManager, but it would change all URLs causing the unwanted side effect of breaking all the user bookmarks.

drigolin commented 1 year ago

Or a kind of service priority. First service matching first serving.

majuca commented 1 year ago

Why don't add a priority parameter in the application.xml like:

<service id="page" ... Priority="2" />
<service id="geonode" ... Priority="1" />

when calling the function resolveRequest, the serviceIDs should be reordered by priority. If nothing define, like order is defined in application.xml

Otherwise, maybe a parameter could be define in services to define the method you want to use, something like:

Like now, default value:
<services Method="AsOrderDefinition" />

like @drigolin propose
<services Method="AsGetParameterOrder" />

Like I propose
<services Method="AsPriorityParameter" />
ctrlaltca commented 1 year ago

Why don't add a priority parameter in the application.xml like:

I thought about this and it could work for simple cases but it wont' on complex ones, eg. if i have more services and more than one service name conflicts with parameters of the other. Imagine we have an "api" (or "json", "soap") service accepting "page" parameters (for pagination) and a "page" service that may accept a parameter like "api" (or "json", "soap") to return a different representation of the page. It becomes a loop of priorities. Also, from a more generic point of view, the norm on web services is that the service/page get determined by the requested path (the part before the question mark in the query). Prado creates an exception because if you don't use URL rewriting it has a single entrypoint in index.php, so it has to get the requested path from the service parameter to the service (page,Pages.Home). The whole idea that just adding a GET (or even POST) parameter at the end of the query changes the requested service/page sounds quite weird to me. That's why my proposal was to prioritize the parameters extracted from the path, or if the don't exists to use the very first parameter from the GET/POST.

Otherwise, maybe a parameter could be define in services to define the method you want to use, something like:

This adds some complexity, but it's definitely a nice way to keep backwards compatibility in case a new approach breaks something.

majuca commented 1 year ago

I'am agree, I don't imagine to add the service in the middle or at the end of the request and your solution to prioritize the parameters make sens for me. We just need dot keep backward compatibility in case of.