Azure / azure-functions-java-library

Contains annotations for writing Azure Functions in Java
MIT License
43 stars 43 forks source link

Change design to a Method-level annotation model #51

Open brunoborges opened 5 years ago

brunoborges commented 5 years ago

Currently, triggers and bindings are defined at the parameter-level. Given the amount of config parameters that the annotations may require, or that the user may want to set, this approach often makes the code extremely hard to read.

Only one trigger is allowed on a (Java method) function, making Trigger a definitive candidate to be a Method annotation, instead of a method-parameter annotation.

Example of Cosmos DB trigger:

 @FunctionName("cosmosDBMonitor")
    public void cosmosDbProcessor(
        @CosmosDBTrigger(name = "items",
            databaseName = "ToDoList",
            collectionName = "Items",
            leaseCollectionName = "leases",
            reateLeaseCollectionIfNotExists = true,
            connectionStringSetting = "AzureCosmosDBConnection") String[] items,
            final ExecutionContext context ) {
                context.getLogger().info(items.length + "item(s) is/are changed.");
            }

In the example above, it is hard to quickly find where the actual method body starts.

A better approach would be to move the annotation to the method-level:

   @FunctionName("cosmosDBMonitor")
   @CosmosDBTrigger(name = "items", databaseName = "ToDoList",
                                    collectionName = "Items", leaseCollectionName = "leases",
                                    createLeaseCollectionIfNotExists = true, 
                                    connectionStringSetting = "AzureCosmosDBConnection",
                                    parameter = "items")
    public void cosmosDbProcessor(String[] items, final ExecutionContext context ) {
        context.getLogger().info(items.length + "item(s) is/are changed.");
    }

Moving the annotation to the method level, requires a new way to bind the trigger and the input object. This can be done in two ways:

  1. Add a new annotation parameter called parameter where the user defines the name of the method parameter that will take the input
  2. Add a new method-parameter annotation type to link the trigger with the input object.

The code snippet above covers option 1.

For option 2, an example would be:

   @FunctionName("cosmosDBMonitor")
   @CosmosDBTrigger(name = "items", databaseName = "ToDoList",
                                    collectionName = "Items", leaseCollectionName = "leases",
                                    createLeaseCollectionIfNotExists = true, 
                                    connectionStringSetting = "AzureCosmosDBConnection")
    public void cosmosDbProcessor(@TriggerObject String[] items, final ExecutionContext context ) {
        context.getLogger().info(items.length + "item(s) is/are changed.");
    }

In the above example for option 2, a new annotation called @TriggerObject is defined to bind the trigger with the method-parameter.

This structure provides two benefits:

  1. Prevents developers from attempting to add two triggers to the same method, and only finding out if this works or note after they try to run on Azure Functions (whether local or on Azure).
  2. Makes the function method body easier to read.

The same approach should be considered for other types: Bindings, and Outputs.

Request for feedback: @JonathanGiles, @pragnagopa, @asavaritayal, @eduardolaureano, @jeffhollan

pragnagopa commented 5 years ago

cc: @paulbatum

paulbatum commented 5 years ago

My initial reaction to this proposal is:

  1. This is a breaking change that feels hard to justify at this point when you consider how much pain our Java preview users encounter every time we make a break, and that we are trying to stabilize the experience in preparation for general availability.
  2. I worry about inconsistency of having trigger annotations at the method level, while outputs are at the parameter level. You mentioned the possibility of moving output annotations to method level as well, but I'm struggling to see how this could work well considering the fact that you can have multiple outputs.
brunoborges commented 5 years ago

@paulbatum: few comments.

  1. It is likely possible to keep compatibility. Annotations may be configured to be applied to one or multiple element types.
  2. For outputs, it is possible to have multiple output annotations at method level, and then each binding to different parameters, annotated accordingly with a config-less annotation as I showed on the second proposal.
jeffhollan commented 5 years ago

I worry more about handling annotations differently in Java purely for readability. I wonder if a potential feature request and pattern we could think about for multiple languages would be allowing you to pass in an options object instead of having to define parameters inline. So could keep consistency and existing patterns but sometime post-GA enabling something like:

   public CosmosDbTriggerOptions cosmosDbOptions = ...

   @FunctionName("cosmosDBMonitor")

    public void cosmosDbProcessor(@CosmosDBTrigger(cosmosDbOptions) String[] items, final ExecutionContext context ) 
    {
        context.getLogger().info(items.length + "item(s) is/are changed.");
    }

Something like above may have added benefits of making it really easy to share config between multiple output bindings to the same source.

Regardless though I think may be better ways to solve readability than making attributes method level

brunoborges commented 5 years ago

@jeffhollan understand your concern, although what you provide as example does not work in Java and unlikely to every be implemented in the language in the future due to how annotations work

paulbatum commented 5 years ago

@brunoborges I might be missing something, but isnt it quite fragile to have method level attributes that are then mapped to parameters by name? As far as I can tell, if you rename the parameter the attributes would break.