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

should not double encode blank node primaryKey #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

should not double encode blank node primaryKey #3

wants to merge 1 commit into from

Conversation

baozuo
Copy link

@baozuo baozuo commented Apr 21, 2017

This is useful in the scenario of updating nested object properties.

@baozuo
Copy link
Author

baozuo commented Apr 21, 2017

hmm, no idea what should I do to pass the checks

@lnshi
Copy link
Owner

lnshi commented Apr 26, 2017

@zuobaojing Thanks for your PR, and sorry for this late reply.

Actually I think there is no reason you want to have the blank node at top level or global scope, probably the lib should consider adding the restriction to totally forbid the user pass in the _: prefixed primaryKey.

As to the nested object properties, I believe the lib already recursively handle this by generating the blank node, as code here: Util.js#L91.

FYI:

@baozuo
Copy link
Author

baozuo commented May 3, 2017

Hi @lnshi
My PR was actually a workaround for the use case of updating a nested object or removing a key from a nested object. The workaround is not intend to be a final fix to issues of these two scenarios, but I didn't figure out a correct/prefect method to resolve the issue.

For example I have the following entity:

{
  "primaryKey": "primary-key",
  "name": "This is my name",
  " properties": {
    "key1": "value1",
    "key2": "value2"
  }
}

Update scenario

Since there are no update API in cayley, when I need to update the entity by changing value of properties.key1 to value111, I need to first remove the old triple and then insert a new triple. That is:

  1. call client.delete API with body: { "primaryKey": "primary-key", "properties": {"key1": "value1"} } to remove the old quad
  2. call client.write API with body: { "primaryKey": "primary-key", "properties": {"key1": "value111"} } to add the new quad

ISSUE: the triple primary-key properties _:BN@<primary-key>.<properties> would also be removed after performing the first step. User could not get key2 until the second step is performed.

Removal scenario

In the case of removing "key1": "value1", I need to:

  1. call client.delete API with body: { "primaryKey": "primary-key", "properties": {"key1": "value1"} } to remove the old triple
    1. call client.write API with body: { "primaryKey": "primary-key", "properties": "_:BN@<primary-key>.<properties>" to add back the triple, which should not be removed

My fix was to workaround the issue this way:

Update scenario

  1. call client.delete API with body: { "primaryKey": "_:BN@<primary-key>.<properties>", "key1": "value1" } to remove the old quad
  2. call client.write API with body: { "primaryKey": "_:BN@<primary-key>.<properties>", "key2": "value111" } to add the new quad

Removal scenario

  1. call client.delete API with body: { "primaryKey": "_:BN@<primary-key>.<properties>", "key1": "value1" } to remove the old quad

Appreciate if you could point me to the right direction in case I missed anything.

@lnshi
Copy link
Owner

lnshi commented Jun 12, 2017

Ah, partial update concern.

@zuobaojing Can you help to add the test case to prove that after you remove the { "primaryKey": "primary-key", "properties": {"key1": "value1"} } then you cannot get key2 case first? Actually I doubt this a bit. If it is like that, then lets figure out the solution. Thanks.

@dennwc Any comments?

@baozuo
Copy link
Author

baozuo commented Sep 28, 2017

@lnshi sorry for late response, we decided not to use this lib earlier months ago, and I stopped working on the testing.
Since I have posted the detailed scenarios/repro steps up there, anyone who might be interested please go ahead and fix the issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants