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

Add document:create#55

Merged
jenow merged 38 commits into
3-devfrom
KZL-1350-document-create
Mar 5, 2020
Merged

Add document:create#55
jenow merged 38 commits into
3-devfrom
KZL-1350-document-create

Conversation

@y-abs
Copy link
Copy Markdown
Contributor

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

What does this PR do ?

This PR initiate the document controller and implements the create 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.
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 createDocument {
    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", "Yoann");
            content.put("nickname", "El angel de la muerte que hace el JAVA");

            ConcurrentHashMap<String, Object> options = new ConcurrentHashMap<>();
            options.put("_id", "some-id");
            options.put("waitForRefresh", true);

            kuzzle.getDocumentController().create("nyc-open-data", "yellow-taxi", content, options)
            .get();
            System.out.println("New document added to the yellow-taxi collection!");
        }  catch (Exception e) {
            e.printStackTrace();
        }

        kuzzle.disconnect();
    }
};

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

Other changes

Update .travis.yml -> Add a stage for the Unit Tests

Boyscout

remove useless import

@y-abs y-abs removed the wip label Feb 7, 2020
Comment thread doc/3/controllers/document/index.md Outdated
final String collection,
final ConcurrentHashMap<String, Object> document,
final String _id,
final ConcurrentHashMap<String, Object> options) throws NotConnectedException, InternalException
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.

A couple weeks ago, @jenow and I decided to amend our confluence document detailing how optional parameters should be handled by SDKs. Long story short: overloads must be used for methods taking only 1 optional argument, and option objects should only be used when there are 2 or more options.

This has been decided because this greatly simplifies the API exposed by SDKs, as well as our code base.

Comment thread doc/3/controllers/document/index.md Outdated

| Option | Type | Description
| ---------- | ----------- | ---------------------------------------------------------------------------------- |
| `refresh` | string | If set to `wait_for`, Kuzzle will wait for the persistence layer to finish indexing|
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.

New SDKs have a waitForRefresh (boolean) option instead of that "refresh = 'wait_for'" one. This is easier to understand, and we'll probably make that update in Kuzzle's API in the future.

Check the query method in this SDK.

.put("action", "create")
.put("_id", _id)
.put("body", document)
.put("options", 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.

that's not how options should be handled, check the query method

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.

Obviously, that was a mistake, sorry

document.put("nickname", "El angel de la muerte que hace el JAVA");

ConcurrentHashMap<String, Object> options = new ConcurrentHashMap<>();
options.put("refresh", "wait_for");
Copy link
Copy Markdown
Contributor

@scottinet scottinet Feb 7, 2020

Choose a reason for hiding this comment

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

if you put the option, then you need to verify that it is properly passed to the API object being sent to kuzzle (and if my remarks above are right, then it isn't)

Comment thread doc/3/controllers/document/index.md Outdated
Comment thread doc/3/controllers/document/index.md Outdated
Comment thread doc/3/controllers/document/index.md Outdated
@y-abs y-abs requested review from Aschen, jenow and scottinet February 17, 2020 15:07
final KuzzleMap query = new KuzzleMap();
final KuzzleMap _options = KuzzleMap
.from(options);
final String _id = _options.getString("_id") == null ? UUID.randomUUID().toString() : _options.getString("_id");
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 don't have to generate the document ID on the client side.

final String _id) throws NotConnectedException, InternalException {
return this.create(index, collection, document, _id, false);
final ConcurrentHashMap<String, Object> options = new ConcurrentHashMap<>();
options.put("_id", UUID.randomUUID().toString());
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.

Same, you don't have to generate the ID on client side

return this.create(index, collection, document, _id, false);
final ConcurrentHashMap<String, Object> options = new ConcurrentHashMap<>();
options.put("_id", UUID.randomUUID().toString());
options.put("waitForRefresh", false);
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.

The default value for waitFoRefresh must handled only on Kuzzle side

## Arguments

```java
public CompletableFuture<ConcurrentHashMap<String, Object>> create(
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 put the 2 possible signatures here

@y-abs y-abs requested a review from Aschen February 20, 2020 14:11
Comment thread doc/3/controllers/document/index.md Outdated
| ------------------ | -------------------------------------------- | --------------------------------- |
| `index` | <pre>string</pre> | Index |
| `collection` | <pre>string</pre> | Collection |
| `content` | <pre>ConcurrentHashMap<String, Object></pre> | Content of the document to create |
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.

Should be document, as in the signature and the options should also be present in the tab

Comment thread doc/3/controllers/document/index.md Outdated

---

# 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.

Why h1 title for this?

Comment thread doc/3/controllers/document/index.md Outdated

| Arguments | Type | Description |
| ------------------ | -------------------------------------------- | --------------------------------- |
| `id` | <pre>string</pre> (optional) | Document identifier. Auto-generated if not specified |
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.

The correct type is String

@y-abs y-abs requested a review from scottinet February 26, 2020 16:28
Comment thread doc/3/controllers/document/create/index.md Outdated
document.put("firstname", "John");
document.put("lastname", "Smith");

kuzzle.getDocumentController().create("nyc-open-data", "yellow-taxi", document)
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 do the snippet test on something from the result as well as for the other methods?

Also 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)

y-abs and others added 2 commits February 27, 2020 10:16
@y-abs y-abs requested a review from Aschen February 28, 2020 14:13
final String index,
final String collection,
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 id and waitForRefresh

@y-abs y-abs force-pushed the KZL-1350-document-create branch from 76ea0f7 to fbbea5c Compare March 2, 2020 10:16
@y-abs y-abs requested a review from jenow March 4, 2020 15:36
@jenow jenow merged commit ec454a5 into 3-dev Mar 5, 2020
@jenow jenow deleted the KZL-1350-document-create branch March 5, 2020 09:59
@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