Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Improve control flow in SurrealQL by introducing RETURN breaking and block expressions #3239

Closed
2 tasks done
JonahPlusPlus opened this issue Dec 31, 2023 · 1 comment
Labels
feature New feature or request triage This issue is new

Comments

@JonahPlusPlus
Copy link

JonahPlusPlus commented Dec 31, 2023

Note: This issue has been upgraded to an RFC.

Is your feature request related to a problem?

In a typical SurQL query, the results of all statements are returned in an array.
If you want to return the result of a particular statement, you can put the statements into a RETURN block.
However, this has a quirk: RETURN does not break out a block/transaction.
This appears to make it work like how it does at the top-level (it can be prefixed to a statement/expression to return it, but the statement/expression is returned regardless, so it doesn't really do anything).
This means that there isn't a way to do early returns with IF-ELSE statements, instead returns need to be repeatedly nested.
For example, here is a query I made in SurQL (see the JS alternative below and my reasons against it):

RETURN {
    LET $sess = type::thing("session", $session_id);

    RETURN IF !$sess.id {
        RETURN { Err: "MissingSession" };
    } ELSE {
        LET $server = type::thing($server_tb, $server_id);

        RETURN IF !$server.id {
            RETURN { Err: "InvalidServer" };
        } ELSE {
            LET $owner = IF $server_tb == "server" {
                RETURN $server.owner;
            } ELSE IF $server_tb == "user" {
                RETURN $server.id;
            } ELSE {
                THROW "Invalid table for server record."
            };

            RETURN IF $owner != $sess.user {
                RETURN { Err: "InvalidOwner" };
            } ELSE {
                LET $server_data = IF $server_tb == "server" {
                    RETURN $server.data;
                } ELSE IF $server_tb == "user" {
                    RETURN $server.personal_server;
                } ELSE {
                    THROW "Invalid table for server record."
                };

                LET $match = (SELECT VALUE channels[WHERE name = $channel_name] FROM ONLY $server_data);

                RETURN IF array::len($match) == 0 {
                    LET $message_channel = (CREATE ONLY message_channel:ulid() SET nsfw = $nsfw);

                    LET $channel = (CREATE ONLY channel:ulid() SET name = $channel_name, ty = $message_channel.id RETURN id).id;

                    UPDATE $server_data SET channels += $channel;

                    RETURN { Ok: $channel };
                } ELSE {
                    RETURN { Err: "ChannelExists" };
                };
            };
        };
    };
};

As you can see, the more conditions that are encountered, the more nested the query becomes.
Perhaps this can be avoided by using THROW statements, but I found the approach of mirroring Result<T, E> easier to handle in my Rust code (since I can just deserialize it in one go).
I need the different error types to throw different API errors.

Describe the solution

Let's start off with an example of what I would like that query to look like and then identify the changes necessary to make it possible:

RETURN {
    LET $sess = type::thing("session", $session_id);

    IF !$sess.id {
        RETURN { Err: "MissingSession" };
    };

    LET $server = type::thing($server_tb, $server_id);

    IF !$server.id {
        RETURN { Err: "InvalidServer" };
    };

    LET $owner = IF $server_tb == "server" {
        $server.owner;
    } ELSE IF $server_tb == "user" {
        $server.id;
    } ELSE {
        RETURN { Err: "InvalidServer" };
    };

    IF $owner != $sess.user {
        RETURN { Err: "InvalidOwner" };
    }

    LET $server_data = IF $server_tb == "server" {
        $server.data
    } ELSE IF $server_tb == "user" {
        $server.personal_server
    } ELSE {
        RETURN { Err: "InvalidServer" };
    };

    LET $match = (SELECT VALUE channels[WHERE name = $channel_name] FROM ONLY $server_data);

    IF array::len($match) == 0 {
        RETURN { Err: "ChannelExists" };
    };

    LET $message_channel = (CREATE ONLY message_channel:ulid() SET nsfw = $nsfw);

    LET $channel = (CREATE ONLY channel:ulid() SET name = $channel_name, ty = $message_channel.id RETURN id).id;

    UPDATE $server_data SET channels += $channel;

    { Ok: $channel }
};

This looks much better (pats back). There are two changes done:

  1. RETURN statements go through blocks and break execution.
  2. Blocks have implicitly returned values.

(Edit: I guess IF-ELSE statements now returning their block expressions also counts as a change)

Notice, that the second rule implies that the first RETURN is unnecessary, the block already returns a value.

Now to go into detail about these changes:

RETURN behavior

The RETURN statement acts just like does in other languages and returns from the current function.

A SurrealQL query can then be described as a sequence of procedures. That means that a RETURN statement doesn't exit the entire query, but only the current procedure. It explains how you could write a query like the above and append another statement which will appear in a separate result.

To help understand what I mean by a sequence of procedures, you can read this:

CREATE foo SET bar = "biz";

SELECT * from foo;

as this:

FUNCTION 0() {
  CREATE foo SET bar = "biz"
}

FUNCTION 1() {
  SELECT * from foo
}

and the result [0(), 1()].

Block Expressions

Just to clarify how block expressions work:
The final statement/expression of a block is the returned value of the block.
If there is a semicolon at the end, the final statement is NONE.
(This is important in Rust because of type checking between branches in if-else statements, but for SurQL it may not matter since it is dynamically typed)

Implementation

I wouldn't mind giving a shot at implementing this. I first wanted to make sure that this is the direction the project is okay with moving towards.

Alternative methods

I made a JS version:

DEFINE FUNCTION fn::create_message_channel($session_id: string, $server: record, $channel_name: string, $nsfw: bool) {
    RETURN function($session_id, $server, $channel_name, $nsfw) {
        const [session_id, server, channel_name, nsfw] = arguments;
        const { Query } = surrealdb;
        const { meta } = surrealdb.functions;
        let session = new Record("session", session_id);
        
        if (await surrealdb.value(`!${session}.id`)) {
            return { Err: "MissingSession" };
        }

        if (await surrealdb.value(`!${server}.id`)) {
            return { Err: "InvalidServer" };
        }

        let server_tb = meta.tb(server);
        let owner;
        let server_data;
        switch (server_tb) {
            case "server":
                owner = await surrealdb.value(`${server}.owner`);
                server_data = await surrealdb.value(`${server}.data`);
                break;
            case "user":
                owner = server;
                server_data = await surrealdb.value(`${server}.personal_server`);
                break;
            default:
                return { Err: "InvalidServer" };
        }

        let user = await surrealdb.value(`${session}.user`);

        if (user.id != owner.id) {
            return { Err: "InvalidOwner" };
        }

        const MATCH_QUERY = new Query(
            "SELECT VALUE channels[WHERE name = $channel_name] FROM ONLY $server_data",
            { channel_name, server_data }
        );

        let matches = await surrealdb.query(MATCH_QUERY);

        if (matches.length != 0) {
            return { Err: "ChannelExists" };
        }

        const MESSAGE_CHANNEL_QUERY = new Query(
            "CREATE ONLY message_channel:ulid() SET nsfw = $nsfw RETURN id",
            { nsfw }
        );

        let message_channel = (await surrealdb.query(MESSAGE_CHANNEL_QUERY)).id;

        const CHANNEL_QUERY = new Query(
            "CREATE ONLY channel:ulid() SET name = $channel_name, ty = $message_channel RETURN id",
            { channel_name, message_channel }
        );

        let channel = (await surrealdb.query(CHANNEL_QUERY)).id;

        await surrealdb.query(new Query(
            "UPDATE $server_data SET channels += $channel",
            { server_data, channel }
        ));

        return { Ok: channel };
    };
};

The problem was that this was far slower (300-400 vs 1500-2000 microseconds, give or take).
I realize that this is due to JS being a more complex language.
So rather than try to make this faster, SurQL should just be a bit more flexible.

When making this JS implementation, I noticed a couple things that would be nice in SurQL, namely switch-case (or maybe Rust's match statement?) and variable declarations (without initialization).

SurrealDB version

ALL VERSIONS

Contact Details

No response

Is there an existing issue for this?

  • I have searched the existing issues

Code of Conduct

  • I agree to follow this project's Code of Conduct
@JonahPlusPlus JonahPlusPlus added feature New feature or request triage This issue is new labels Dec 31, 2023
@JonahPlusPlus
Copy link
Author

I realize that there needs to be clarification on how this would work with transactions.
Transactions in their current state are pretty convoluted. They only work at the top-level and provide no warning about this when used in lower scopes.
For example, the following doesn't work as expected:

{
  BEGIN;
  CREATE foo SET bar = "biz";
  CANCEL;
}

The expected result would be that the CREATE statement doesn't commit because of the CANCEL, but when executed we see that it does create a new record, and that the BEGIN/CANCEL/COMMIT statements have no apparent effect.

Anyways, the problem at hand is how RETURN statements change the result of a transaction. If returns break execution, it wouldn't allow calling COMMIT or CANCEL and getting a result.

IMO, the most intuitive solution is to make transactions work in blocks and make RETURN act like a CANCEL that breaks execution.

A couple examples:

BEGIN;
CREATE foo SET bar = "biz";
RETURN CREATE foo SET bar = "boo";
# This RETURN does nothing (unlike before, where it would make it the single return value of the transaction
COMMIT;

Result:

[
  [{id:"foo:f09uja09sdj0asd", bar:"biz"}],
  [{id:"foo:adsad0h80jfe09j", bar:"boo"}]
]
{
  BEGIN;
  CREATE foo SET bar = "biz";
  RETURN CREATE foo SET bar = "boo"; # Here, the RETURN cancels the transaction and returns the CREATE
  COMMIT; # This block doesn't return a value
}

Result:

[
  [{id:"foo:adsad0h80jfe09j", bar:"boo"}]
]
{
  BEGIN;
  CREATE foo SET bar = "biz";
  CREATE foo SET bar = "boo";
  COMMIT;
  true # the block returns true
}

Result: [true]

Edge cases:
What if a later statement depends on the result of a statement inside a canceled transaction?
This is currently covered:

BEGIN;
LET $foo = CREATE foo SET bar = "biz";
CANCEL;
RETURN $foo;

$foo produces a phantom result, a record that never existed and is worthless outside of the canceled transaction. To me, this represent the biggest flaw of this syntax: the transaction has a scope that is not expressed, but is instead merged into the current scope.
An improved syntax would be explicit about the scope:

BEGIN {
  LET $foo = CREATE foo SET bar = "biz";
  CANCEL false; # an optional return parameter
  $foo
}

Here, the scope is explicit and variables are not leaked outside. It also allows for an implicit commit as the final value. CANCEL and COMMIT would have an optional return value for setting the value of the transaction.

Anyways, I guess this does warrant an RFC considering how many breaking changes would be needed.

@JonahPlusPlus JonahPlusPlus closed this as not planned Won't fix, can't repro, duplicate, stale Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request triage This issue is new
Projects
None yet
Development

No branches or pull requests

1 participant