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

Bug: INFO... query parsing error (RUST SDK) #3984

Closed
2 tasks done
fokklz opened this issue May 5, 2024 · 14 comments · Fixed by #4595
Closed
2 tasks done

Bug: INFO... query parsing error (RUST SDK) #3984

fokklz opened this issue May 5, 2024 · 14 comments · Fixed by #4595
Assignees
Labels
bug Something isn't working topic:net This is related to the server and networking topic:rust This is related to the Rust embedded library topic:surrealql This is related to the SurrealQL query language

Comments

@fokklz
Copy link

fokklz commented May 5, 2024

Describe the bug

When trying to run a query with INFO FOR the parsing fails & nothing else happens.
The function just seems to die.

Steps to reproduce

I've created a Repository to allow for simpler reproduction of the issue.

  1. run: git clone https://github.com/fokklz/surreal-bug-report-parse-error.git
  2. run: surreal start memory
  3. open project root and run: cargo run

Expected behaviour

I'd expect a similar Response to the one from inside surrealist. If run there, i get:

INFO FOR SCOPE users;

{
	tokens: {}
}

INFO FOR NS;

{
	databases: {
		...
	},
	tokens: {},
	users: {}
}

SurrealDB version

1.4.2-1.5.0 (cargo & exe)

Contact Details

chat@fokklz.dev

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
@fokklz fokklz added bug Something isn't working triage This issue is new labels May 5, 2024
@fokklz fokklz changed the title Bug: INFO FOR... query parsing error Bug: INFO FOR... query parsing error (RUST SDK) May 5, 2024
@risha700
Copy link

risha700 commented May 5, 2024

investigating broken code from 1.42 upgrade rust sdk

Tried few alternate supported channels.
in-memory => passes info for table but fails info for ns,root
ws => fail all
http => passes

INFO FOR TABLE tablename;
INFO FOR NS;
INFO FOR ROOT;

and deadlock error as follows:

ERROR surrealdb::api::engine::remote::ws::native] Failure { code: -32700, message: "Parse error" }

BTW v1.4.0 works well

@DelSkayn DelSkayn added topic:net This is related to the server and networking and removed triage This issue is new labels May 7, 2024
@fokklz
Copy link
Author

fokklz commented May 9, 2024

it seems also to be happening for the USE statement.
i've created a branch for that as well but it's simply an additional query.

@fokklz fokklz changed the title Bug: INFO FOR... query parsing error (RUST SDK) Bug: INFO / USE... query parsing error (RUST SDK) May 9, 2024
@fokklz
Copy link
Author

fokklz commented May 14, 2024

Still present in 1.5.0.

The issue with the USE command seems to be resolved

@fokklz fokklz changed the title Bug: INFO / USE... query parsing error (RUST SDK) Bug: INFO... query parsing error (RUST SDK) May 14, 2024
@Dhghomon
Copy link
Contributor

Dhghomon commented May 16, 2024

I'm seeing the same behaviour too. Interestingly if you add the STRUCTURE keyword after INFO (e.g. INFO FOR NS STRUCTURE;) it doesn't hang up anymore but says it can't parse it and asks if it's missing a semicolon. (Just in case that context helps anyone working on this)

@phughk
Copy link
Contributor

phughk commented May 21, 2024

@LivingLimes has mentioned they will work on this

@LivingLimes
Copy link
Contributor

I am running the current repository locally using cargo. The current version is SurrealDB command-line interface and server 2.0.0+20240516.22193888.dirty for macos on aarch64.

@fokklz Running INFO FOR SCOPE users; works for me, returning

[{ tokens: {  } }]

Running INFO FOR NS; also works for me, returning

[{ databases: { test: 'DEFINE DATABASE test' }, tokens: {  }, users: {  } }]

I notice that the query you mentioned in the issue was actually INFO FOR NS;; (with two semi colons), but I'm not sure if this is a valid statement. Can you confirm this?

@risha700 I'm a new contributor here. Could you explain what you mean by running on difference channels? I'm running surrealdb locally using cargo watch -x 'run --no-default-features --features storage-mem,http,scripting -- start --log trace --user root --pass root memory' and running surreal queries using cargo run -- sql --endpoint http://localhost:8000 --username root --password root --namespace test --database test. and I was able to execute all your queries successfully. I'm trying to reproduce the ws error. Could I get some guidance on this?

@Dhghomon Running INFO FOR NS STRUCTURE; returns:

[{ databases: [], tokens: [], users: [] }]

tldr: I am unable to reproduce this issue on the latest main branch of this repository. If more details could be added to assist with reproduction, that would help.

@fokklz
Copy link
Author

fokklz commented May 22, 2024

@LivingLimes well it's not about the cli. It's about the SDK, I linked a repo, clone and run

Did not try directly using main tho. It's the published version of the crate

@fokklz
Copy link
Author

fokklz commented May 22, 2024

When i run it using surreal's latest install script so 1.5.0 on Windows i get the following, no matter if with or without ;

image

I do not have v2 running localy, u have a download? but i guess its possibly fixed there then @LivingLimes

@risha700
Copy link

risha700 commented May 22, 2024

@LivingLimes Thank you for taking time to investigate at first, the issue seem to be super nested due to surrealdb versatility to run. As much as I like the idea, the sql behavior is inconsistent across native installation, in-memory, behind docker on unix.

Regarding INFO statement parse error

I can confirm it exists while
using:

  • rust sdk 1.5.0
  • websocket over docker image surrealdb/surrealdb:v1.5.0 or older
  • or local memory # if authenticated before you call info, deadlock
  • or http,cli # works smoothly behind docker

The issue reproduction in a nutshell

// from the docs

use std::env;
use surrealdb::engine::any;
use surrealdb::opt::auth::Root;

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    // Use the endpoint specified in the environment variable or default to `memory`.
    // This makes it possible to use the memory engine during development but switch it
    // to any other engine for deployment.
    let endpoint = env::var("SURREALDB_ENDPOINT").unwrap_or_else(|_| "ws://localhost:8000".to_owned());
    env_logger::init_from_env(env_logger::Env::default().default_filter_or("debug"));
    // Create the Surreal instance. This will create `Surreal<Any>`.
    let db = any::connect(endpoint).await?;
    let _s = db.signin(Root{
        username: "root",
        password: "pass",
    }).await.expect("Can't authenticate SurrealDB Root");
    // Specify the namespace and database to use
    db.use_ns("test").use_db("test").await?;
    // let ns = db.query(surrealdb::sql::statements::InfoStatement::Ns).await.expect("cant get response"); //fails as well
    let ns = db.query("info for nsr").await.expect("cant get response");
    println!("NS {:#?}", ns);
    Ok(())
}

while cargo deps

[dependencies]
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
surrealdb = { version = "1.5.0", features = ["kv-mem"] }
tokio = { version = "1.37.0", features = ["full"] }
env_logger = "0.11.3"

As a work around for now running surrealdb/surrealdb:nightly works fine, seem to be with engine i guess.

@fokklz for info for ns, has a remedy use session::ns

@LivingLimes
Copy link
Contributor

@fokklz @risha700 Thanks for the extra information. From what I can see, I can't reproduce this when using the code from main, which is consistent with what @risha700 said about using the nightly build as a workaround.

@phughk Can you ask if anyone on the team is aware of changes that would have fixed this issue? If we're not sure what could have fixed the issue, I can do some investigation to figure out if this issue has actually been addressed in main or if it was only by chance that these issues seem to be resolved by the nightly build.

@risha700 I am also able to reproduce your issue with auth, but I think it makes sense to create a new issue for this so it can be tracked independently of this ticket. I can help create this ticket a bit later.

@phughk phughk added the topic:surrealql This is related to the SurrealQL query language label May 23, 2024
@phughk
Copy link
Contributor

phughk commented May 23, 2024

Screenshot 2024-05-23 at 13 37 14 Thanks for the repo reproduction @fokklz ! Can confirm 1.4.0 works, 1.4.2 and 1.5.0-beta.1 do not work This is looking like it's related to the parser rewrite @DelSkayn or perhaps CBOR @kearfy ?

@kearfy
Copy link
Member

kearfy commented May 23, 2024

Hey everyone! It's not CBOR related nor is it related to the parser rewrite, as the new parser is only available behind a feature flag. For the launch of Surrealist however, we had to add a new STRUCTURE keyword to the INFO FOR statement. This then also resulted in a bump of Revision, and moving some macros. I'm positive that somewhere in this process something has broken, I'm not certain what, however.

I think the second PR has the biggest chance of being the culprit, it could however also be the third PR.

CC @DelSkayn

@kearfy kearfy added the topic:rust This is related to the Rust embedded library label May 23, 2024
@LivingLimes
Copy link
Contributor

I've been looking into this one since I'm unsure if it's immediately apparent to the team where the issue might be coming from. It has also been helping me get acquainted with the codebase. I should be done investigating in the next few days.

@LivingLimes
Copy link
Contributor

Issue identification

I have identified the issue around usage of INFO FOR SCOPE users;. Note: all my links are to my branch that is based off tag v1.5.1.

The error is being thrown when trying to deserialise the InfoStatement enum. This failure is happening because the serialised bytes (prior to being sent through the web socket) use revision 1 of the InfoStatement object and the variant indices it references are as though there none of the variants from revision 2. Therefore, when deserialising the bytes, the variant index does not always point to the correct variant because the revision 2 object may contain more variants.

This is a little difficult to explain with words so I've created a branch with some tests to illustrate this. I also created an integration test to indicate failure when running the INFO FOR SCOPE users query.

To summarise, I think the problem is in either the serialisation of the payload before it is sent through the web socket or it is in the deserialisation of the message after it is received by the web socket connection to be parsed by the query parser.

Some solutions

  1. Understand why InfoStatement is being parsed with revision 1 in the first place and see if we can parse it as revision 2. However, unless it should instead be parsed as revision 2, this doesn't resolve the issue long term though as this should technically be a valid state.
  2. Update the serialisation code to be aware of the variants that are not available in its revision so that on deserialisation, the correct variant will be targeted.
  3. Update the deserialisation code to ignore all variants that are not part of the current revision.

I think either (1) or (3) are the ideal solution making the serialisation aware of variants that aren't present in a revision prevents us from serialising by using a different revision of the struct or enum.

Other notes ...

I've had some difficulty debugging this and I'm not sure if it's because of metaprogramming or generics in a separate crate (revision) are involved so I'll either take a break before returning to this or (hopefully) get some pointers on how to move forward. @phughk @kearfy

@Dhghomon I haven't closely investigated your finding that calling INFO FOR SCOPE user STRUCTURE; seems to throw a different error, but I don't see it in the documentation and was wondering if it's supposed to be in the public API?

@DelSkayn DelSkayn self-assigned this Jun 14, 2024
@DelSkayn DelSkayn mentioned this issue Aug 23, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working topic:net This is related to the server and networking topic:rust This is related to the Rust embedded library topic:surrealql This is related to the SurrealQL query language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants