Skip to content
This repository was archived by the owner on Sep 28, 2022. It is now read-only.

Add document:update#59

Merged
y-abs merged 34 commits into
3-devfrom
KZL-1350-document-update
Mar 12, 2020
Merged

Add document:update#59
y-abs merged 34 commits into
3-devfrom
KZL-1350-document-update

Conversation

@y-abs
Copy link
Copy Markdown
Contributor

@y-abs y-abs commented Feb 21, 2020

What does this PR do ?

This PR implements the document:update method with its unit tests.

How should this be manually tested?

Clone this branch and run unit tests
./gradlew test

When it succeed, compile it

./gradlew jar

Initiate another java project by adding the compiled SDK as a dependency.

Then, run Kuzzle, create an index nyc-open-data and a yellow-taxi collection in the admin console. Create a document with id some-id and content:

{
        "name" : "random"
}

Finally, run this code

import io.kuzzle.sdk.*;
import io.kuzzle.sdk.Options.KuzzleOptions;
import io.kuzzle.sdk.Options.Protocol.WebSocketOptions;
import io.kuzzle.sdk.Protocol.WebSocket;

import java.util.concurrent.ConcurrentHashMap;

public class updateDocument {
    private static Kuzzle kuzzle;

    public static void main(String[] args) {
        WebSocketOptions opts = new WebSocketOptions();
        opts.setAutoReconnect(true).setConnectionTimeout(42000);

        try {
            WebSocket ws = new WebSocket("localhost", opts);

            kuzzle = new Kuzzle(ws, (KuzzleOptions) null);

            kuzzle.connect();

            ConcurrentHashMap<String, Object> content = new ConcurrentHashMap<>();
            content.put("name", "not-so-random");

            ConcurrentHashMap<String, Object> options = new ConcurrentHashMap<>();
            options.put("source", true);

            ConcurrentHashMap<String, Object> response =
            kuzzle.getDocumentController().update("nyc-open-data", "yellow-taxi", "some-id", content, options)
            .get();
            System.out.println("Document updated!");
            System.out.println(response);
	}  catch (Exception e) {
            e.printStackTrace();
        }

        kuzzle.disconnect();
    }
};

You should see the document updated in your collection.
You can test without options as well

Other changes

Update subscribe documentation -> Remove non existing snippets
Remove class DocumentOptions and add a CreateOptions class

@y-abs y-abs self-assigned this Feb 21, 2020
curl -XPUT kuzzle:7512/nyc-open-data/yellow-taxi
after:
template: default
expected: "id: 'some-id'" No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this test can pass. Can you add documentation snippet tests to travis to make sure of that?

Copy link
Copy Markdown
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same problems than in your other PR (#55)

@y-abs y-abs force-pushed the KZL-1350-document-update branch from 8c1e0a2 to b97884c Compare February 26, 2020 14:26
@y-abs y-abs requested a review from scottinet February 26, 2020 16:28
Comment thread doc/3/controllers/document/update/index.md Outdated
ConcurrentHashMap<String, Object> response =
kuzzle.getDocumentController().update("nyc-open-data", "yellow-taxi", "some-id", content)
.get();
System.out.println(response); No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the result of the println in comment?
It helps developer to better apprehend the structure of the response object they have. We try to do this for every method (Look the Javascript SDK and Doc)

.put("body", document)
.put("_id", id)
.put("waitForRefresh", _options != null ? _options.getBoolean("waitForRefresh") : null)
.put("retryOnConflict", _options != null ? _options.getNumber("retryOnConflict") : 0)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should let Kuzzle handle default values

Suggested change
.put("retryOnConflict", _options != null ? _options.getNumber("retryOnConflict") : 0)
.put("retryOnConflict", _options != null ? _options.getNumber("retryOnConflict") : null)

Copy link
Copy Markdown
Contributor Author

@y-abs y-abs Feb 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When its null Kuzzle throw an error

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, but this sounds like a bug in Kuzzle. So I fixed it: kuzzleio/kuzzle#1579

Aschen's point still stands.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks!
So unfortunately, until it's released, the snippet test will fail

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'm releasing kuzzle 2.1.1 then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yoann-Abbes > kuzzle 2.1.1 released

@y-abs y-abs requested a review from Aschen February 28, 2020 14:13
final String collection,
final String id,
final ConcurrentHashMap<String, Object> document,
final ConcurrentHashMap<String, Object> options)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a DocumentOptions class containing waitForRefresh, retryOnConflict and source

Copy link
Copy Markdown
Contributor

@Aschen Aschen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After @jenow request 👍

@y-abs y-abs requested a review from jenow March 5, 2020 15:58
final String collection,
final String id,
final ConcurrentHashMap<String, Object> document,
final DocumentOptions options)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree with that name: that option class can only be passed to methods having a use for all its exposed options, namely waitForRefresh, retryOnConflict and source. These last 2 in particular are only relevant for update (and possibly mUpdate) methods.

You have to design 1 option class per "use" (in term of functionalities), meaning that all options must have meaning for a particular method. If you use a "DocumentOption" class for methods having no use for the retryOnConflict option for instance, then this can be hard for users to understand.

Rename this to UpdateOptions, DocumentUpdateOptions, or whatever is more fitting for this method.

Copy link
Copy Markdown
Contributor

@scottinet scottinet Mar 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just saw that DocumentOptions already existed prior to this PR, and that it contains an id option for methods such as document.create.

This makes my point even more relevant: the update method has no use for that id option, and this can even be very confusing for users, as the update method already takes an "id" argument.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed 👍

Comment thread doc/3/controllers/document/update/index.md Outdated
Comment thread doc/3/controllers/document/update/index.md Outdated
Comment thread doc/3/controllers/realtime/subscribe/index.md Outdated
@scottinet scottinet mentioned this pull request Mar 6, 2020
@y-abs y-abs requested a review from scottinet March 11, 2020 13:34
@y-abs y-abs merged commit f83c01d into 3-dev Mar 12, 2020
@y-abs y-abs deleted the KZL-1350-document-update branch March 12, 2020 11:53
@jenow jenow mentioned this pull request May 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants