You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "spmallette (via GitHub)" <gi...@apache.org> on 2025/08/29 12:51:45 UTC
[PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
spmallette opened a new pull request, #3194:
URL: https://github.com/apache/tinkerpop/pull/3194
https://issues.apache.org/jira/browse/TINKERPOP-3182
`Path` objects were being forcibly detached by reference in GraphSON v1/v2 which put it at odds with v1 untyped. Inclusion or removal of properties should be controlled via `materializeProperties` only.
There's still a bit of weirdness here for Javascript particularly if you note the assertions in our tests. That should be resolved separately with a breaking change: [TINKERPOP-3186](https://issues.apache.org/jira/browse/TINKERPOP-3186).
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#issuecomment-3237761620
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#discussion_r2310707825
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.js:
##########
@@ -79,10 +79,19 @@ class Edge extends Element {
this.inV = inV;
this.properties = {};
if (properties) {
- const keys = Object.keys(properties);
- for (let i = 0; i < keys.length; i++) {
- const k = keys[i];
- this.properties[k] = properties[k].value;
+ if (Array.isArray(properties)) {
+ // Handle array of Property objects
+ for (let i = 0; i < properties.length; i++) {
+ const prop = properties[i];
+ this.properties[prop.key] = prop.value;
+ }
+ } else {
+ // Handle object format as before
+ const keys = Object.keys(properties);
+ for (let i = 0; i < keys.length; i++) {
+ const k = keys[i];
+ this.properties[k] = properties[k].value;
+ }
Review Comment:
Nit: can simplify
```suggestion
if (Array.isArray(properties)) {
// Handle array of Property objects
properties.forEach(prop => this.properties[prop.key] = prop.value);
} else {
// Handle object format as before
Object.keys(properties).forEach(k => this.properties[k] = properties[k].value);
```
Edit: nevermind, linter doesn't like this for some reason
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#discussion_r2310707825
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.js:
##########
@@ -79,10 +79,19 @@ class Edge extends Element {
this.inV = inV;
this.properties = {};
if (properties) {
- const keys = Object.keys(properties);
- for (let i = 0; i < keys.length; i++) {
- const k = keys[i];
- this.properties[k] = properties[k].value;
+ if (Array.isArray(properties)) {
+ // Handle array of Property objects
+ for (let i = 0; i < properties.length; i++) {
+ const prop = properties[i];
+ this.properties[prop.key] = prop.value;
+ }
+ } else {
+ // Handle object format as before
+ const keys = Object.keys(properties);
+ for (let i = 0; i < keys.length; i++) {
+ const k = keys[i];
+ this.properties[k] = properties[k].value;
+ }
Review Comment:
Nit: can simplify
```suggestion
if (Array.isArray(properties)) {
// Handle array of Property objects
properties.forEach(prop => this.properties[prop.key] = prop.value);
} else {
// Handle object format as before
Object.keys(properties).forEach(k => this.properties[k] = properties[k].value);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#discussion_r2310707825
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.js:
##########
@@ -79,10 +79,19 @@ class Edge extends Element {
this.inV = inV;
this.properties = {};
if (properties) {
- const keys = Object.keys(properties);
- for (let i = 0; i < keys.length; i++) {
- const k = keys[i];
- this.properties[k] = properties[k].value;
+ if (Array.isArray(properties)) {
+ // Handle array of Property objects
+ for (let i = 0; i < properties.length; i++) {
+ const prop = properties[i];
+ this.properties[prop.key] = prop.value;
+ }
+ } else {
+ // Handle object format as before
+ const keys = Object.keys(properties);
+ for (let i = 0; i < keys.length; i++) {
+ const k = keys[i];
+ this.properties[k] = properties[k].value;
+ }
Review Comment:
Nit: can simplify
```suggestion
if (Array.isArray(properties)) {
// Handle array of Property objects
properties.forEach((prop) => (this.properties[prop.key] = prop.value));
} else {
// Handle object format as before
Object.keys(properties).forEach((k) => (this.properties[k] = properties[k].value));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette merged PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#discussion_r2310707480
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.js:
##########
@@ -79,10 +79,19 @@ class Edge extends Element {
this.inV = inV;
this.properties = {};
if (properties) {
- const keys = Object.keys(properties);
- for (let i = 0; i < keys.length; i++) {
- const k = keys[i];
- this.properties[k] = properties[k].value;
+ if (Array.isArray(properties)) {
Review Comment:
no. doing this here in `Edge` preserves the map structure for `properties` that was there before irrespective of what the serializer decides to send in the constructor. `Vertex` and `VertexProperty` just had a direct assignment which just accepted what the serializer gave it (with all the weirdness that comes with that), so i left that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#issuecomment-3238274680
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#issuecomment-3238190368
VOTE +1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#discussion_r2310685201
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.js:
##########
@@ -79,10 +79,19 @@ class Edge extends Element {
this.inV = inV;
this.properties = {};
if (properties) {
- const keys = Object.keys(properties);
- for (let i = 0; i < keys.length; i++) {
- const k = keys[i];
- this.properties[k] = properties[k].value;
+ if (Array.isArray(properties)) {
Review Comment:
Should this same logic apply for `Vertex`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3182 Fixed inconsistency in Path GraphSON serialization [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3194:
URL: https://github.com/apache/tinkerpop/pull/3194#discussion_r2310707825
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.js:
##########
@@ -79,10 +79,19 @@ class Edge extends Element {
this.inV = inV;
this.properties = {};
if (properties) {
- const keys = Object.keys(properties);
- for (let i = 0; i < keys.length; i++) {
- const k = keys[i];
- this.properties[k] = properties[k].value;
+ if (Array.isArray(properties)) {
+ // Handle array of Property objects
+ for (let i = 0; i < properties.length; i++) {
+ const prop = properties[i];
+ this.properties[prop.key] = prop.value;
+ }
+ } else {
+ // Handle object format as before
+ const keys = Object.keys(properties);
+ for (let i = 0; i < keys.length; i++) {
+ const k = keys[i];
+ this.properties[k] = properties[k].value;
+ }
Review Comment:
Nit: can simplify
```suggestion
if (Array.isArray(properties)) {
// Handle array of Property objects
properties.forEach(prop => this.properties[prop.key] = prop.value);
} else {
// Handle object format as before
Object.keys(properties).forEach(k => this.properties[k] = properties[k].value);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org