Skip to content

Conversation

@mnd999
Copy link
Contributor

@mnd999 mnd999 commented Jun 12, 2025

Error codes for graph types.

This work is likely to be feature flagged for some time, so we don't want to merge it to the public docs yet, but we do need to review the errors.

@mnd999 mnd999 added dev The default branch. DO NOT MERGE cypher-25 labels Jun 12, 2025
@mnd999 mnd999 marked this pull request as ready for review June 16, 2025 09:14
@mnd999 mnd999 force-pushed the graph-type-errors branch from 85e42a1 to d27c275 Compare June 16, 2025 09:15
Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

I haven't looked at the implementation PR but I did have some thoughts on the error messages from just looking at them in isolation.

Status: 0 open comment in this group and 1 in the next

@mnd999 mnd999 force-pushed the graph-type-errors branch from 9eff486 to af24129 Compare July 2, 2025 14:14
----
ALTER CURRENT GRAPH TYPE SET {
(p:Person => {name :: STRING}),
CONSTRAINT FOR (p:Person =>) REQUIRE p.name IS NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want a second example where the node elem type and constraint have different properties (to show that it's not only a problem when the properties are the same)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need to, if somebody is looking at this page, they already have the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, but I just felt that since both the node and the relationship example had the same property it might be nice to also show some other cases that would fail

ALTER CURRENT GRAPH TYPE SET {
    (p:Person => {name :: STRING}),
    CONSTRAINT FOR (p:Person) REQUIRE p.ssn IS NOT NULL
}


Fix to:
ALTER CURRENT GRAPH TYPE SET {
    (p:Person => {name :: STRING, ssn:: ANY NOT NULL})
}

(this both have another property and doesn't say we expect the label to be identifying in the constraint syntax)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need any more examples, most of the error codes do not have any.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, cause they didn't have time to add any when doing the initial docs, they want there to be examples for all codes (I've been asked to add examples when I updated codes previously) so that's not really a metric to go after

Copy link
Contributor

Choose a reason for hiding this comment

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

but if you don't want new/additional example then update the existing one instead: I just want some more variation in the examples and not have the very same case in both the node and relationship version (just switching which constraint type is where)

@mnd999 mnd999 force-pushed the graph-type-errors branch 2 times, most recently from 7d5bab3 to e25ea42 Compare July 17, 2025 07:57
Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

New round of comments

Status: only open comments are from the groups above

----
ALTER CURRENT GRAPH TYPE SET {
(p:Person => {name :: STRING}),
CONSTRAINT FOR (p:Person =>) REQUIRE p.name IS NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, but I just felt that since both the node and the relationship example had the same property it might be nice to also show some other cases that would fail

ALTER CURRENT GRAPH TYPE SET {
    (p:Person => {name :: STRING}),
    CONSTRAINT FOR (p:Person) REQUIRE p.ssn IS NOT NULL
}


Fix to:
ALTER CURRENT GRAPH TYPE SET {
    (p:Person => {name :: STRING, ssn:: ANY NOT NULL})
}

(this both have another property and doesn't say we expect the label to be identifying in the constraint syntax)

Copy link
Collaborator

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

Many editorial comments, some of which require fixing the description in the codebase. When these are addressed, I'll build it locally and read it again. I also need to regenerate the index file to add the new codes.

Copy link
Collaborator

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

Looks good. A few minor comments.

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

Noticed when double checking the messages and the impl updates

@renetapopova renetapopova self-requested a review July 25, 2025 11:34
Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

Review of new errors (and a brief look through the old)

Status: two open comments

[source,cypher]
----
ALTER CURRENT GRAPH TYPE ALTER {
CONSTRAINT c1 FOR (s:Student) REQUIRE s.studentId :: STRING
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Gowri have (multiple times) stumbled on the inline key constraints it might be nice with an example of that as well

Given
ALTER CURRENT GRAPH TYPE SET {
    (:Label => :ToBeRemoved {prop :: STRING IS KEY})
}

Then:
ALTER CURRENT GRAPH TYPE ALTER {
    (:Label => {prop :: STRING IS KEY})
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you have it for DROP, but I think Gowri have had more questions on it for ALTER 🤷

Comment on lines 44 to 49
}
ALTER CURRENT GRAPH TYPE ADD {
Copy link
Contributor

Choose a reason for hiding this comment

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

would we need some indication that these are two queries and not one containing two alter clauses? 🤔 (since this would fail parsing on the second ALTER I'd assume)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How is this normally done?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would guess either separating them into two code blocks or having ; after them, but I'd double check how the docs team would want it

@mnd999 mnd999 force-pushed the graph-type-errors branch from 2f73662 to 4cea064 Compare October 13, 2025 16:02
@mnd999 mnd999 force-pushed the graph-type-errors branch from 4cea064 to f2ada6a Compare October 13, 2025 16:09
Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

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

Next group of comments

@mnd999
Copy link
Contributor Author

mnd999 commented Oct 14, 2025

@renetapopova I've updated this with the remaining set of error codes, and there is a couple of outstanding questions for docs.

@renetapopova
Copy link
Collaborator

@renetapopova I've updated this with the remaining set of error codes, and there is a couple of outstanding questions for docs.

I'll take a look next week.

@renetapopova
Copy link
Collaborator

Is this PR for 2025.10?

@mnd999
Copy link
Contributor Author

mnd999 commented Oct 21, 2025

@renetapopova No, I don't think this is ready for release yet.

}
----

=== Remove a KEY constraint using ALTER
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're adding and not removing a KEY constraint 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is autogenerated. You don't need to update it.

Copy link
Collaborator

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

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

I added some suggestions and a placeholder for the version.

Co-authored-by: Reneta Popova <[email protected]>
Co-authored-by: Therese Magnusson <[email protected]>
@neo4j-docops-agent
Copy link
Collaborator

This PR includes documentation updates
View the updated docs at https://neo4j-docs-status-codes-341.surge.sh

New pages:

Updated pages:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants