surrealdb / surrealdb.java

SurrealDB SDK for Java
https://surrealdb.com
Apache License 2.0
67 stars 21 forks source link

Fix JSON Object deserialization. #44

Closed Zyrenth closed 1 year ago

Zyrenth commented 1 year ago

Fixed JSON Object deserialization in SurrealWebSocketConnection. Errors occurred when trying to create/select/update/etc a database record with a specified ID, see more here. This fixes those errors occurring, but after some testing, I encountered other problems such as: when trying to select a record with a specified ID, it would just return an empty Array. EDIT: I also encountered problems with creating a new record with a specified ID.

Overall more testing and fixing need to be done for other parts of the code.

If there is any confusion, just ask me. Thanks to @phughk for the explanations and help!

phughk commented 1 year ago

Nice! Thank you so much! I was wondering if we could also have a test that verifies it. Something like the following in the websocket connection class test

    private static final String localAddr = "ws://localhost";
    private static final int localPort = 8000;

    @Test
    public void canHandleSingleObjectResult() {
        SurrealWebSocketConnection connection = new SurrealWebSocketConnection(localAddr, localPort, false);
        connection.connect(1);
        SyncSurrealDriver driver = new SyncSurrealDriver(connection);
        Map<String, String> leslie = Map.of(
            "name" , "Leslie",
            "surname","Lamport"
        );
        driver.create("person:leslie", leslie );
        List<Map> vals = driver.select("person:leslie", Map.class);
        assertEquals(1, vals.size());
    }

    @Test
    void canHandleMultiObjectResult() {
        SurrealWebSocketConnection connection = new SurrealWebSocketConnection(localAddr, localPort, false);
        connection.connect(1);
        SyncSurrealDriver driver = new SyncSurrealDriver(connection);
        Map<String, String> leslie = Map.of(
            "name" , "Leslie",
            "surname","Lamport"
        );
        driver.create("person:leslie", leslie );
        Map<String,String> barbara = Map.of(
            "name", "Barbara",
            "surname", "Liskov"
        );
        List<Map> vals = driver.select("person", Map.class);
        assertEquals(2, vals.size());
        Assertions.fail("expected");
    }

I tried running it with gradle integrationTest and it actually didn't run!? There is something wrong with the filter.

If you have time for it that is. If you don't I can create an issue instead and merge as is :)

Zyrenth commented 1 year ago

Nice! Thank you so much! I was wondering if we could also have a test that verifies it. Something like the following in the websocket connection class test

    private static final String localAddr = "ws://localhost";
    private static final int localPort = 8000;

    @Test
    public void canHandleSingleObjectResult() {
        SurrealWebSocketConnection connection = new SurrealWebSocketConnection(localAddr, localPort, false);
        connection.connect(1);
        SyncSurrealDriver driver = new SyncSurrealDriver(connection);
        Map<String, String> leslie = Map.of(
            "name" , "Leslie",
            "surname","Lamport"
        );
        driver.create("person:leslie", leslie );
        List<Map> vals = driver.select("person:leslie", Map.class);
        assertEquals(1, vals.size());
    }

    @Test
    void canHandleMultiObjectResult() {
        SurrealWebSocketConnection connection = new SurrealWebSocketConnection(localAddr, localPort, false);
        connection.connect(1);
        SyncSurrealDriver driver = new SyncSurrealDriver(connection);
        Map<String, String> leslie = Map.of(
            "name" , "Leslie",
            "surname","Lamport"
        );
        driver.create("person:leslie", leslie );
        Map<String,String> barbara = Map.of(
            "name", "Barbara",
            "surname", "Liskov"
        );
        List<Map> vals = driver.select("person", Map.class);
        assertEquals(2, vals.size());
        Assertions.fail("expected");
    }

I tried running it with gradle integrationTest and it actually didn't run!? There is something wrong with the filter.

If you have time for it that is. If you don't I can create an issue instead and merge as is :)

No problem, thanks. Also, I tried running gradle integrationTest on the original repo but it failed as well, so most likely it's an existing issue. I will probably only have time over the weekend to finish it.

phughk commented 1 year ago

Yeah, easy! Theres no rush and we really appreciate the work you are doing! Thank you! :)

Zyrenth commented 1 year ago

Yeah, easy! Theres no rush and we really appreciate the work you are doing! Thank you! :)

So I was working on different projects and I haven't had time to implement the other fixes. So let's merge this and I'll open another issue for those problems and if I'll have time to implement those other fixes I'll do it.

phughk commented 1 year ago

I would rather we have test coverage, especially for this issue. Will merge once we have tests.

phughk commented 1 year ago

I have added this pr as accepted for hacktoberfest @Zyrenth, if you are participating :) Thanks again for the contribution!