Add test for validating REST API routing & service order

[timvisee]

Is your feature request related to a problem? Please describe.
The order of services we define for our REST API is important. If ordering is incorrect, there can be problems resolving a route due to pattern collisions.

Read more about this here: #3543.

The PR #3544 fixes this problem, but we want to make sure that ordering isn't accidentally broken afterwards.

Describe the solution you'd like
To prevent this, we would like to add a test that asserts routing and ordering of services is correct.

We imagine the following:

• create app definition for actix (if required)
• set up a few dummy requests, such as POST /collections/some/points/scroll
• get route pattern for request, possibly using match_pattern
• ensure pattern matches what we expect

We imagine setting up a few dummy requests, such as to scroll, and asserting that the right route pattern is matched.

Most importantly, we want this new test to fail if ordering in #3544 would be reverted.

Describe alternatives you've considered
We could also add some integration test to call each read-only endpoint asserting the responses. But that would require quite a bit of work, and it would be less complete than testing actix routing and service ordering directly.

Additional context
I tried to implement a proof of concept for this test, but sadly that was not sufficient. In the snippet below we define the app having routes, and we define a request. But the match_pattern function to select what route pattern is matched does not know about our app. There does not seem to be a way to connect the two together.

#[cfg(test)]
mod tests {
    use actix_web::{http::header::ContentType, test, App};

    use super::*;

    #[actix_web::test]
    async fn test_routing_order() {
        let app = test::init_service(App::new()
                .service(index)
                .configure(config_collections_api)
                .configure(config_snapshots_api)
                .configure(config_update_api)
                .configure(config_cluster_api)
                .configure(config_service_api)
                .configure(config_search_api)
                .configure(config_recommend_api)
                .configure(config_discovery_api)
                .configure(config_shards_api)
                .service(scroll_points)
                .service(count_points)
                .service(get_point)
                .service(get_points)).await;

        let req = test::TestRequest::post()
            .app_data(app)
            .uri("/collections/test_collection/points/scroll")
            .insert_header(ContentType::json())
            .to_srv_request();

        assert_eq!(req.match_pattern().unwrap(), "/collections/{collection}/points/scroll");
    }
}

Also, actix web has some testing facilities that may be helpful:

• https://actix.rs/docs/testing/
• https://docs.rs/actix-web/latest/actix_web/test/index.html

[generall]

/bounty $200

[algora-pbc]

💎 $200 bounty created by Qdrant
🙋 If you start working on this, comment /attempt #3547 along with your implementation plan
👉 To claim this bounty, submit a pull request that includes the text /claim #3547 somewhere in its body
📝 Before proceeding, please make sure you can receive payouts in your country
💵 Payment arrives in your account 2-5 days after the bounty is rewarded
💯 You keep 100% of the bounty award
🙏 Thank you for contributing to qdrant/qdrant!

👉 Add a bounty • Share on socials

Attempt	Started (GMT+0)	Solution
🔴 @haruncurak	Feb 6, 2024, 4:07:58 PM	WIP
🟢 @kemkemG0	Feb 7, 2024, 4:35:13 AM	#3556

[haruncurak]

/attempt #3547

Algora profile	Completed bounties	Tech	Active attempts	Options
@haruncurak	4 bounties from 3 projects	TypeScript, Elixir	#3522	Cancel attempt

[kemkemG0]

/attempt #3547

Options

• Cancel my attempt

[Lalit3716]

@timvisee So I have been trying this but it looks like there isn’t any way to write a test similar to the one you gave as proof of concept at the moment due to actix limitations.
But I thought of a different approach we could take here, basically this is essentially an ordering error which could be avoided if user registered services in correct order.

So maybe we could write a function to handle this responsibility of ordering our urls correctly and then we could write our tests for that function?

Sudo Code:-

all_services = [(url, handler), …]; // Note here ordering doesn’t matter
ordered_services = order_services(all_services);
for service in ordered_services {
    cfg.service(service);
}

And then we could just test our order_services helper with some dummy test cases to validate our routing?

Of course order_services here will make sure to put /collections/aliases above /collections/{id} and that’s what we will be testing.

[timvisee]

basically this is essentially an ordering error which could be avoided if user registered services in correct order.

You're right. That's basically what the test was meant to validate.

I do see value in configuring everything in a single array, which itself must be ordered correctly. That would at least give us a single place to configure things, in which we can describe specific ordering with comments.

I see that you're suggesting a tuple of the URL and service handler above. I'm assuming we only include the URL here so we can do some testing. But I don't think I like this approach. It makes things more complex than they need to be, and we don't even verify whether the specified URL actually matches the service.

So in my opinion, having a single array in which we specify the services in correct order is good enough. Without explicit testing, because we cannot easily do this.

Thank you very much for thinking along! That's much appreciated.

[Lalit3716]

I agree that having tuples of URL and service handlers will make code a little bit complex.

Yeah I was thinking of having a tuple of (URL, Handlers) so that we can test out whether the handlers are ordered correctly by looking at their routes.

Defining an array with correct order of services should work for now but still we are leaving our code on assumptions in this way and have no actual tests to actually make sure the order. I think we could maybe have a defined mapping between our URLs and our Handlers at least maybe a HashMap?

[generall]

This problem was resolved by the access-token based control in RBAC implementation