-
Notifications
You must be signed in to change notification settings - Fork 493
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
Update to not use deprecated index and constraint syntax #2385
Update to not use deprecated index and constraint syntax #2385
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have problem with building this branch, maybe you could help me out.
Found some more places to update:
CREATE CONSTRAINT ON ... ASSERT
in SchemasTest.java
db.index.fulltext
in SignatureTest.java
and signatures.csv
@@ -231,7 +231,8 @@ private void exportSchema(PrintWriter out) { | |||
private List<String> exportIndexes() { | |||
return db.executeTransactionally("CALL db.indexes()", Collections.emptyMap(), result -> result.stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change CALL db.index()
here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no this method is the one causing https://trello.com/c/KJUMznPU/2751-apoc-failures-due-to-removal-of-dbindexes and can not yet be removed without breaking things
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I see
is it the create or query methods? cause it is only create/drop that are being removed, the query ones are still valid |
Ok, these are not create so should be fine then |
Hi @Hunterness what is the status of this one? |
@conker84 Not really sure, I don't know why it doesn't build but I would like to see a green build before merging to know I have fixed things properly (I had issues with building things locally in general so I can't even check it myself) |
Left the case in `exportIndexes` as that can not currently be replaced by SHOW INDEXES
switching from procedures to commands
2192047
to
01b1dc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. I get a few test failures when building this locally. But as far as I can tell it has nothing to do with these changes.
@conker84 I managed got get it to build locally, and all tests I have changed seems to work now so it should be ready to be merged (and hope nothing else breaks) |
They are currently in progress of being removed and are therefore in need of being updated