watson-developer-cloud / java-sdk

:1st_place_medal: Java SDK to use the IBM Watson services.
http://watson-developer-cloud.github.io/java-sdk/
Apache License 2.0
593 stars 532 forks source link

Assistant v1 - Can't remove NextStep from Dialog Node #1153

Closed paulombweber closed 3 years ago

paulombweber commented 3 years ago

While using updateDialogNode trying to update my node, I also want to remove the NextStep saved previously, but since Behaviour is required by Builder, I can't send a empty DialogNodeNextStep, so my choice is to send null wich is ignored when the requestBody is being assembled and the next step isn't correctly removed.

I expected that I could send either send null to newNextStep, or send a empty DialogNodeNextStep so that the Assistant backend treat it and remove the next step.

To reproduce create a Assistant, set some node with a next step to anywhere and try to remove it using updateDialogNode

Sdk Version 8.5.0, I tried to update that but on the newest version there's no DialogNodeOutputGeneric.Builder, so I can't build my outputs, so if the fix is on the sdk I would need that to be added back too.

paulombweber commented 3 years ago

I forked the assistant and removed the validation if (updateDialogNodeOptions.newNextStep() != null) {

it fixed the issue but since the source seems to be generated it probably isn't the right way to fix it. Also, it could unintentionally remove the nextStep in some cases.

kevinkowa commented 3 years ago

Hi @paulombweber, what version did you fork?

paulombweber commented 3 years ago

Hi @paulombweber, what version did you fork?

I used the master branch for the latest, I managed to fix breaking changes on my application code to work with the new version.

paulombweber commented 3 years ago

@kevinkowa I've pushed the code on my forked repo so that you can better understand. https://github.com/paulombweber/java-sdk/commit/9fbdb41f85c3e2f43766f7fcc3dea4fa6b58c3fd

I'm not sure this is the correct fix, because if someone uses the SDK as a PATCH request(only sending what did change) it will cause problems.

kevinkowa commented 3 years ago

well, the problem here is that if you look at the documentation (https://cloud.ibm.com/apidocs/assistant/assistant-v1#updatedialognode), assistant doesn't allow a null value for it. it only has the following Allowable values: [get_user_input,skip_user_input,jump_to,reprompt,skip_slot,skip_all_slots]

paulombweber commented 3 years ago

@kevinkowa Yes, the behaviour can't be null, that's ok. But how can I remove the next step then? my commit allows to send null to the nextStep, but it'll affect users that expect the SDK to work as a PATCH.

kevinkowa commented 3 years ago

I'm investigating this issue, I'll keep you updated!

tiagoboeing commented 3 years ago

@kevinkowa any news about that?

kevinkowa commented 3 years ago

We believe this is happening across multiple SDKs, so we are checking into possible solutions. I'll keep this issue updated when we learn more about the issue.

vini0012 commented 3 years ago

@kevinkowa I have the same problem, any news about it?

kevinkowa commented 3 years ago

We are looking into it 👍

kevinkowa commented 3 years ago

Hi, thank you for your patience. You can now try the new version 9.1.1 which includes a fix for this issue. We introduced new models and a new functions so this is achievable.

Example of usage:

// set the properties that you want to override.
UpdateDialogNode updateDialogNode =
    new UpdateDialogNode.Builder()
        .description(newDialogNodeDescription)
        .nextStep(null)
        .dialogNode(newDialogNodeName)
        .build();
 // convert to map using asPatch()
Map<String, Object> body = updateDialogNode.asPatch();

// Provide the options which includes the body (map)
// dialogNode is the name of the node before you update it.
UpdateDialogNodeNullableOptions updateDialogNodeNullableOptions =
    new UpdateDialogNodeNullableOptions.Builder()
        .workspaceId(workspaceId)
        .dialogNode(dialogNodeName)
        .body(body)
        .build();
DialogNode response =
    service.updateDialogNodeNullable(updateDialogNodeNullableOptions).execute().getResult();

If there are any questions, feel free to create a new issue!

paulombweber commented 3 years ago

This solved my issue, but just for the record, .asPatch isn't a patch, you should rename it to .asPut, since as REST standard a patch you should only send what you want to change, and this method does the exatc opposite, it just get the object to map, so if you have any property previously saved and you doesn't inform it, you'll overrade it.