generate POJO methods getPrimaryIdentifier and getAdditionalIdentifiers #103

aygold92 · 2019-06-12T20:59:11Z

Issue #, if available: #95

Description of changes:

An important assumption in the POJO file is that IdentifierPaths will always begin with /properties/, which I believe this will always be the case.

Example:

Given schema

{
    "typeName": "AWS::Goldband::Band",
    "description": "An example resource schema demonstrating some basic constructs and validation rules.",
    "sourceUrl": "[email protected]:aws-cloudformation/aws-cloudformation-rpdk.git",
    "definitions": {
        "SubProperty": {
            "type": "object",
            "properties": {
                "NestedA": {
                    "type": "string"
                },
                "NestedB": {
                    "type": "integer"
                }
            }
        },
        "OtherSub": {
            "type": "object",
            "properties": {
                "OtherNestedA": {
                    "type": "string"
                },
                "OtherNestedB": {
                    "type": "string"
                }
            }
        }
    },
    "properties": {
        "TPSCode": {
            "description": "A TPS Code is automatically generated on creation and assigned as the unique identifier.",
            "type": "string",
            "pattern": "^[A-Z]{3,5}[0-9]{8}-[0-9]{4}$"
        },
        "NestedProperty": {
            "$ref": "#/definitions/SubProperty"
        },
        "OtherNestedProperty": {
            "$ref": "#/definitions/OtherSub"
        }
    },
    "readOnlyProperties": [
        "/properties/TPSCode"
    ],
    "primaryIdentifier": [
        "/properties/TPSCode",
        "/properties/NestedProperty/NestedA"
    ],
    "additionalIdentifiers": [
        [
            "/properties/NestedProperty/NestedB"
        ],
        [
            "/properties/OtherNestedProperty/OtherNestedA",
            "/properties/OtherNestedProperty/OtherNestedB"
        ]
    ],
    "handlers": {
        "create": {
            "protocolVersion": "1.0",
            "runtimeType": "Lambda",
            "permissions": [
                "initech:CreateReport"
            ]
        },
        "read": {
            "protocolVersion": "1.0",
            "runtimeType": "Lambda",
            "permissions": [
                "initech:DescribeReport"
            ]
        },
        "update": {
            "protocolVersion": "1.0",
            "runtimeType": "Lambda",
            "permissions": [
                "initech:UpdateReport"
            ]
        },
        "delete": {
            "protocolVersion": "1.0",
            "runtimeType": "Lambda",
            "permissions": [
                "initech:DeleteReport"
            ]
        },
        "list": {
            "protocolVersion": "1.0",
            "runtimeType": "Lambda",
            "permissions": [
                "initech:ListReports"
            ]
        }
    }
}

we generate

// This is a generated file. Modifications will be overwritten.
package com.aws.goldband.band;

import com.fasterxml.jackson.annotation.JsonProperty;
import java.util.Map;
import java.util.List;
import java.util.ArrayList;
import java.util.Set;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Data;
import lombok.NoArgsConstructor;
import org.json.JSONObject;


@Data
@Builder
@AllArgsConstructor
@NoArgsConstructor
public class ResourceModel {

    public static final String TYPE_NAME = "AWS::GoldBand::Band";

    public static final String IDENTIFIER_KEY_TPSCODE = "/properties/TPSCode";
    public static final String IDENTIFIER_KEY_NESTEDPROPERTY_NESTEDA = "/properties/NestedProperty/NestedA";
    public static final String IDENTIFIER_KEY_NESTEDPROPERTY_NESTEDB = "/properties/NestedProperty/NestedB";
    public static final String IDENTIFIER_KEY_OTHERNESTEDPROPERTY_OTHERNESTEDA = "/properties/OtherNestedProperty/OtherNestedA";
    public static final String IDENTIFIER_KEY_OTHERNESTEDPROPERTY_OTHERNESTEDB = "/properties/OtherNestedProperty/OtherNestedB";

    @JsonProperty("TPSCode")
    private String tPSCode;

    @JsonProperty("NestedProperty")
    private SubProperty nestedProperty;

    @JsonProperty("OtherNestedProperty")
    private OtherSub otherNestedProperty;

    public JSONObject getPrimaryIdentifier(){
        final JSONObject identifier = new JSONObject();
        if (this.getTPSCode() != null) {
            identifier.append(IDENTIFIER_KEY_TPSCODE, this.getTPSCode());
        }

        if (this.getNestedProperty() != null && this.getNestedProperty().getNestedA() != null) {
            identifier.append(IDENTIFIER_KEY_NESTEDPROPERTY_NESTEDA, this.getNestedProperty().getNestedA());
        }

        // only return the identifier if it can be used, i.e. if all components are present
        return identifier.length() == 2 ? identifier : null;
    }

    public List<JSONObject> getAdditionalIdentifiers(){
        final List<JSONObject> identifiers = new ArrayList<JSONObject>();
        if (getIdentifier_NestedB() != null) {
            identifiers.add(getIdentifier_NestedB());
        }
        if (getIdentifier_OtherNestedA_OtherNestedB() != null) {
            identifiers.add(getIdentifier_OtherNestedA_OtherNestedB());
        }
        // only return the identifiers if any can be used
        return identifiers.isEmpty() ? null : identifiers;
    }

    public JSONObject getIdentifier_NestedB(){
        final JSONObject identifier = new JSONObject();
        if (this.getNestedProperty() != null && this.getNestedProperty().getNestedB() != null) {
            identifier.append(IDENTIFIER_KEY_NESTEDPROPERTY_NESTEDB, this.getNestedProperty().getNestedB());
        }

        // only return the identifier if it can be used, i.e. if all components are present
        return identifier.length() == 1 ? identifier : null;
    }

    public JSONObject getIdentifier_OtherNestedA_OtherNestedB(){
        final JSONObject identifier = new JSONObject();
        if (this.getOtherNestedProperty() != null && this.getOtherNestedProperty().getOtherNestedA() != null) {
            identifier.append(IDENTIFIER_KEY_OTHERNESTEDPROPERTY_OTHERNESTEDA, this.getOtherNestedProperty().getOtherNestedA());
        }

        if (this.getOtherNestedProperty() != null && this.getOtherNestedProperty().getOtherNestedB() != null) {
            identifier.append(IDENTIFIER_KEY_OTHERNESTEDPROPERTY_OTHERNESTEDB, this.getOtherNestedProperty().getOtherNestedB());
        }

        // only return the identifier if it can be used, i.e. if all components are present
        return identifier.length() == 2 ? identifier : null;
    }
}

On the POJOs for the subObjects we don't see these methods

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aygold92 · 2019-06-12T21:01:22Z

note that the build works locally but fails here because the RPDK does not have the new schema with primaryIdentifier and additionalIdentifier

rjlohan

We need to update identifier handling in CloudFormation before we merge the new schema to RPDK, and then this can land. 👍

python/rpdk/java/templates/POJO.java

…-pojo

rjlohan · 2019-06-25T23:33:51Z

Need to add some tests to the generated code, and include them in the /templates source that is generated as part of the package. The test can make sure that the correct identifiers from the schema are parsed for example.

aygold92 · 2019-06-26T20:14:18Z

python/rpdk/java/codegen.py

+        contents = template.render(
+            type_name=project.type_name,
+            package_name=self.package_name,
+            properties=pojos["ResourceModel"],


note: these args are there because I was going to try to generate values in order to get the test coverage up. will remove these if we're settled on this being not necessary

rjlohan · 2019-06-27T16:54:13Z

You should add some more tests around the identifier generation; use compound keys, multiple primaryIdentifiers, bad paths, etc. Then, you can fix the build by excluding the ResourceModel POJO class from the generated packages JaCoCo expectations, by adding a line here;

https://github.com/aws-cloudformation/aws-cloudformation-rpdk-java-plugin/blob/master/python/rpdk/java/templates/pom.xml#L157

Like
<exclude>**/ResourceModel*</exclude>

rjlohan · 2019-06-27T16:51:00Z

python/rpdk/java/templates/ResourceModelTest.java

+    public void test_ResourceModel_SimpleSuccess() {
+        final ResourceModel model = ResourceModel.builder().build();
+
+        assertThat(model.getPrimaryIdentifier(), is(not(nullValue())));


Can we add some more complex schemas for testing these additions? If we test the generation here, we can exclude the generated ResourceModel.java POJO from the package coverage expectations.

will do. The problem is that if we can't test the logic anyway (without generating values) there isn't really any good way to verify that this will generate properly for any schema

we have tests which verify the pojo_resolver with different schemas so thats most of it there. The only untested part (automated at least) is the identifier methods, and this is non trivial to test since it would require generating values, or creating some other strategy. I have raised #119 to address this.

I've done manual testing with:

one identifier

compound identifier

multiple additional identifiers

no additional identifiers

up to four levels deep of nesting in an identifierpath

python/rpdk/java/templates/POJO.java

tobywf

What does the use-case look like for this? I get how getPrimaryIdentifier is useful, but am struggling to see how getAdditionalIdentifiers would be used the current form. Since additionalIdentifiers is a list with list/array semantics, intuitively null values should be preserved to preserve indices. Unless you only cared about the first valid one - but then this code could short-circuit earlier.

So without examples how these might be used, I'm really not sure what makes sense here.

python/rpdk/java/codegen.py

python/rpdk/java/templates/POJO.java

python/rpdk/java/templates/ResourceModelTest.java

tobywf · 2019-06-27T22:12:06Z

python/rpdk/java/templates/ResourceModelTest.java

+
+public class ResourceModelTest {
+    @Test
+    public void test_ResourceModel_SimpleSuccess() {


unnecessary test_ prefix

python/rpdk/java/templates/ResourceModelTest.java

python/rpdk/java/codegen.py

python/rpdk/java/templates/POJO.java

…-pojo

aygold92 · 2019-06-28T18:18:20Z

What does the use-case look like for this? I get how getPrimaryIdentifier is useful, but am struggling to see how getAdditionalIdentifiers would be used the current form. Since additionalIdentifiers is a list with list/array semantics, intuitively null values should be preserved to preserve indices. Unless you only cared about the first valid one - but then this code could short-circuit earlier.

So without examples how these might be used, I'm really not sure what makes sense here.

I envision this being used by the read handler, since you can pass any set of identifiers it would allow for a quick validation check and easy access to a set of identifiers.

generate POJO methods getPrimaryIdentifier and getAdditionalIdentifiers

c83f590

rjlohan suggested changes Jun 12, 2019

View reviewed changes

python/rpdk/java/templates/POJO.java Outdated Show resolved Hide resolved

python/rpdk/java/templates/POJO.java Outdated Show resolved Hide resolved

aygold92 added 2 commits June 13, 2019 16:07

fix whitespace

cc71788

Merge remote-tracking branch 'origin/master' into generate-identifier…

249f036

…-pojo

aygold92 added 2 commits June 26, 2019 12:46

add validation for identifiers, generate TestResourceModel file

6cdf659

fixed whitespacing in files

ee72614

aygold92 commented Jun 26, 2019

View reviewed changes

rjlohan reviewed Jun 27, 2019

View reviewed changes

return null for incomplete identifier, exclude ResourceModel in testing

0ccf77b

rjlohan approved these changes Jun 27, 2019

View reviewed changes

tobywf suggested changes Jun 27, 2019

View reviewed changes

aygold92 added 2 commits June 27, 2019 15:30

Merge remote-tracking branch 'origin/master' into generate-identifier…

d93e00d

…-pojo

address comments and fix jinja template whitespacing

68b9435

tobywf approved these changes Jul 11, 2019

View reviewed changes

aygold92 merged commit 8fc5e6a into master Jul 12, 2019

aygold92 deleted the generate-identifier-pojo branch July 12, 2019 20:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generate POJO methods getPrimaryIdentifier and getAdditionalIdentifiers #103

generate POJO methods getPrimaryIdentifier and getAdditionalIdentifiers #103

aygold92 commented Jun 12, 2019 •

edited by tobywf

Loading

aygold92 commented Jun 12, 2019

rjlohan left a comment

rjlohan commented Jun 25, 2019

aygold92 Jun 26, 2019

rjlohan commented Jun 27, 2019

rjlohan Jun 27, 2019

aygold92 Jun 27, 2019 •

edited

Loading

aygold92 Jun 27, 2019 •

edited

Loading

tobywf left a comment

tobywf Jun 27, 2019

aygold92 Jun 27, 2019

aygold92 commented Jun 28, 2019

generate POJO methods getPrimaryIdentifier and getAdditionalIdentifiers #103

generate POJO methods getPrimaryIdentifier and getAdditionalIdentifiers #103

Conversation

aygold92 commented Jun 12, 2019 • edited by tobywf Loading

aygold92 commented Jun 12, 2019

rjlohan left a comment

Choose a reason for hiding this comment

rjlohan commented Jun 25, 2019

aygold92 Jun 26, 2019

Choose a reason for hiding this comment

rjlohan commented Jun 27, 2019

rjlohan Jun 27, 2019

Choose a reason for hiding this comment

aygold92 Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

aygold92 Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

tobywf left a comment

Choose a reason for hiding this comment

tobywf Jun 27, 2019

Choose a reason for hiding this comment

aygold92 Jun 27, 2019

Choose a reason for hiding this comment

aygold92 commented Jun 28, 2019

aygold92 commented Jun 12, 2019 •

edited by tobywf

Loading

aygold92 Jun 27, 2019 •

edited

Loading

aygold92 Jun 27, 2019 •

edited

Loading