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.

Sign up for GitHub

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

Jump to bottom

[NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept default string values #3036

Merged
merged 3 commits into from
Jan 24, 2023
Merged

[NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept default string values #3036

merged 3 commits into from
Jan 24, 2023

Conversation

vga91
Copy link
Collaborator

@vga91 vga91 commented Jul 6, 2022 •
edited
Loading

Fixes #2978

  • Updated SIGNATURE_SYNTAX_ERROR
  • Fixed default string handling
  • Added getDefaultParameterValue to differentiate between plain text and other data types

With these changes the apoc.custom.declareFunction/Procedure should be able to accept any kind of default value, except for float type fixed here.

@vga91 vga91 changed the title Fixes #2978: The apoc.custom.declareProcedure does not accept default… Fixes #2978: The apoc.custom.declareProcedure does not accept default string values Jul 6, 2022
@vga91 vga91 requested review from jexp and Lojjs July 6, 2022 10:21
@nadja-muller nadja-muller assigned AzuObs and Lojjs and unassigned AzuObs Jul 7, 2022
AzuObs
AzuObs approved these changes Jul 11, 2022
View reviewed changes
Copy link
Contributor

@AzuObs AzuObs left a comment

Choose a reason for hiding this comment

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

Good job 🎉

@vga91 vga91 removed the request for review from Lojjs July 13, 2022 12:41
@jexp
Copy link
Member

jexp commented Aug 25, 2022

The reason for originally leaving it out in the parser was that the default signature output doesn't put quotes around strings, which made it too hard to parse for me.

@vga91
Copy link
Collaborator Author

vga91 commented Aug 31, 2022

The reason for originally leaving it out in the parser was that the default signature output doesn't put quotes around strings, which made it too hard to parse for me.

@jexp
Just to be sure, so do you think the changes are ok?
Or should I change them in another way?

Anyway, the pr change the Signature.g4 placed in core, but indeed this file is only called by Signature.java (in full),
therefore since we are not changing core functionality, I think it may be mergeable.

@AzuObs AzuObs removed their assignment Sep 8, 2022
@Lojjs Lojjs assigned AzuObs and unassigned Lojjs Oct 24, 2022
@jexp
Copy link
Member

jexp commented Dec 12, 2022

@Lojjs we should still adjust the grammar file in core. (And possible also fix the signature output in cypher for strings)

@jexp
Copy link
Member

jexp commented Dec 12, 2022

Actually @vga91 @Lojjs we should move the grammar file to extended as it's not used anywhere in core, at least not to my knowledge.

jexp
jexp reviewed Dec 12, 2022
View reviewed changes
full/src/main/java/apoc/custom/Signatures.java Outdated
}
if (v.INT_VALUE() != null) {
final String text = v.INT_VALUE().getText();
return getDefaultParameterValue(type, text, () -> DefaultParameterValue.ntInteger(Integer.parseInt(text)));
Copy link
Member

Choose a reason for hiding this comment

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

Btw. we should fix this as it should be Long.parseLong()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

jexp
jexp reviewed Dec 12, 2022
View reviewed changes
full/src/main/java/apoc/custom/Signatures.java Outdated
}
if (v.FLOAT_VALUE() != null) {
final String text = v.FLOAT_VALUE().getText();
return getDefaultParameterValue(type, text, () -> DefaultParameterValue.ntFloat(Float.parseFloat(text)));
Copy link
Member

Choose a reason for hiding this comment

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

And this Double.parseDouble()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

jexp
jexp approved these changes Dec 12, 2022
View reviewed changes
Copy link
Member

@jexp jexp 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! Please addithe bugfix of the old bug that was there before.

Lojjs added a commit to neo4j/apoc that referenced this pull request Dec 12, 2022
@Lojjs
[wokdlhX3] Move Signature.g4
cd8daa4 
See comment in neo4j-contrib/neo4j-apoc-procedures#3036
Lojjs added a commit that referenced this pull request Dec 12, 2022
@Lojjs
[wokdlhX3] Move Signature.g4
8e5e6c5 
See comment in #3036
@Lojjs
Copy link
Contributor

Lojjs commented Dec 12, 2022

Actually @vga91 @Lojjs we should move the grammar file to extended as it's not used anywhere in core, at least not to my knowledge.

I will move it as part of this PR in dev: neo4j/apoc#261. So if you can do your cherry-pick after that goes in, that would be good. Otherwise there is a risk I will write over your changes to the file by moving it

Lojjs added a commit that referenced this pull request Jan 2, 2023
@Lojjs
[wokdlhX3] Move Signature.g4
7ba6c8a 
See comment in #3036
Lojjs added a commit to neo4j/apoc that referenced this pull request Jan 11, 2023
@Lojjs
[wokdlhX3] Move Signature.g4
96a0eeb 
See comment in neo4j-contrib/neo4j-apoc-procedures#3036
@vga91 vga91 changed the title Fixes #2978: The apoc.custom.declareProcedure does not accept default string values [NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept default string values Jan 24, 2023
@vga91 vga91 merged commit 47f80c4 into neo4j-contrib:4.4 Jan 24, 2023
vga91 added a commit that referenced this pull request Aug 29, 2023
@vga91
[NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept …
ba6792f 
…default string values (#3036)

* [NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept default string values

* [NOID] removed unused imports

* [NOID] changed parseInt/Float to correct parseLong/Double
vga91 added a commit that referenced this pull request Aug 29, 2023
@vga91
[NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept …
9b86e09 
…default string values (#3036) (#3753)

* [NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept default string values

* [NOID] removed unused imports

* [NOID] changed parseInt/Float to correct parseLong/Double
recrwplay pushed a commit to recrwplay/neo4j-apoc-procedures that referenced this pull request Aug 31, 2023
@vga91 @recrwplay
[NOID] Fixes neo4j-contrib#2978: The apoc.custom.declareProcedure doe…
7c8a1ba 
…s not accept default string values (neo4j-contrib#3036) (neo4j-contrib#3753)

* [NOID] Fixes neo4j-contrib#2978: The apoc.custom.declareProcedure does not accept default string values

* [NOID] removed unused imports

* [NOID] changed parseInt/Float to correct parseLong/Double
vga91 added a commit that referenced this pull request Sep 1, 2023
@vga91
[NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept …
2d445b8 
…default string values (#3036) (#3753)

* [NOID] Fixes #2978: The apoc.custom.declareProcedure does not accept default string values

* [NOID] removed unused imports

* [NOID] changed parseInt/Float to correct parseLong/Double
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jexp jexp jexp approved these changes

@AzuObs AzuObs AzuObs approved these changes

Assignees

@AzuObs AzuObs

Labels
extended-functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The apoc.custom.declareProcedure does not accept default string values
4 participants
@vga91 @jexp @Lojjs @AzuObs