Add sql binding java annotations #379
Conversation
lucyzhang929
requested review from
Charles-Gagnon,
MaddyDev and
chlafreniere
as code owners
|
|
||
| <developers> | ||
| <developer> | ||
| <id>LucyZhang</id> |
There was a problem hiding this comment.
This should probably be something generic like "Microsoft"
There was a problem hiding this comment.
Other extensions all seem to be using the developer's names as id:
https://github.com/Azure/azure-functions-rabbitmq-extension/blob/c319042b4297a6beb0c526652c124a1ca4305496/java-library/pom.xml#L40
https://github.com/Azure/azure-functions-signalrservice-extension/blob/bb21bb00d91cc68b3ec9e3d02938be8a1d4d5c36/binding-library/java/pom.xml#L57
There was a problem hiding this comment.
Can you reach out to see if there's a reason they're doing that? That's not normally how we ship stuff so I don't think we should be doing that unless there's a specific reason it needs to be someone's name.
There was a problem hiding this comment.
Actually looking at the XSD it seems the developers field is optional so I'd just remove that - unless again there's a specific requirement for having at least one developer listed.
java-library/src/main/java/com/microsoft/azure/functions/sql/annotation/SQLOutput.java
Show resolved
Hide resolved
java-library/src/main/java/com/microsoft/azure/functions/sql/annotation/SQLOutput.java
Show resolved
Hide resolved
Charles-Gagnon
left a comment
Charles-Gagnon
left a comment
There was a problem hiding this comment.
Should probably remove the developers fields but otherwise looks fine
| /** | ||
| * Text or Stored Procedure. | ||
| */ | ||
| String commandType() default ""; |
There was a problem hiding this comment.
we're defaulting to Text on CSharp, shouldn't we do the same here instead of leaving it empty?
There was a problem hiding this comment.
I tried to use Text as default but when I ran a Java function without the commandType specified, it threw an error:
Microsoft.Azure.WebJobs.Extensions.Sql: The type of the SQL attribute for an input binding must be either CommandType.Text for a direct SQL query, or CommandType.StoredProcedure for a stored procedure.
I'll leave it empty for now but can certainly revisit this.
There was a problem hiding this comment.
Please file an issue so we can track that
Will be creating a separate PR for samples and tests.