Skip to content

Commit

Permalink
fix(go): dates are mistreated as strings (#2730)
Browse files Browse the repository at this point in the history 
Implements the correct code generation flow & runtime logic to correctly
handle jsii date values as *time.Time instances.

Fixes #2659



---

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

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0
  • Loading branch information
@RomainMuller
RomainMuller authored Mar 24, 2021
1 parent b4d334a commit 2ba2ec4
Show file tree
Hide file tree
Showing 16 changed files with 255 additions and 137 deletions.
10 changes: 5 additions & 5 deletions gh-pages/content/specification/6-compliance-report.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
This section details the current state of each language binding with respect to our standard compliance suite.


| number | test | java (100.00%) | golang (70.09%) | Dotnet | Python |
| number | test | java (100.00%) | golang (73.50%) | Dotnet | Python |
| ------ | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------- | -------------------------------------------- | ------ | ------ |
| 1 | asyncOverrides_overrideCallsSuper | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) | ⭕ | ⭕ |
| 2 | [arrayReturnedByMethodCanBeRead]("Array created in the kernel can be queried for its elements") | 🟢 | 🟢 | ⭕ | ⭕ |
Expand All @@ -28,7 +28,7 @@ This section details the current state of each language binding with respect to
| 19 | [doNotOverridePrivates_method_private]("Non public methods on the guest class do not override methods in the host class") | 🟢 | 🟢 | ⭕ | ⭕ |
| 20 | [pureInterfacesCanBeUsedTransparently]("Guest implementation of a pure host interface can be used by host consumers accepting that interface") | 🟢 | 🟢 | ⭕ | ⭕ |
| 21 | [nullShouldBeTreatedAsUndefined]("Null value of target language is treated as undefined by the host") | 🟢 | 🟢 | ⭕ | ⭕ |
| 22 | [primitiveTypes]("All Primitive types are set and read with their respective types") | 🟢 | [🔴](https://github.com/aws/jsii/issues/2659) | ⭕ | ⭕ |
| 22 | [primitiveTypes]("All Primitive types are set and read with their respective types") | 🟢 | 🟢 | ⭕ | ⭕ |
| 23 | [reservedKeywordsAreSlugifiedInClassProperties]("TS code that uses reserved words as class property names get slugified so it is usable in the target language") | 🟢 | ⚪ | ⭕ | ⭕ |
| 24 | [objectIdDoesNotGetReallocatedWhenTheConstructorPassesThisOut]("Ensure the JSII kernel can pass 'this' out to JSII remotes from within the constructor") | 🟢 | 🟢 | ⭕ | ⭕ |
| 25 | [interfaceBuilder]("Seems to be a duplicate of 'propertyOverrides_interfaces'?") | 🟢 | 🟢 | ⭕ | ⭕ |
Expand All @@ -52,7 +52,7 @@ This section details the current state of each language binding with respect to
| 43 | doNotOverridePrivates_property_getter_public | 🟢 | 🟢 | ⭕ | ⭕ |
| 44 | equalsIsResistantToPropertyShadowingResultVariable | 🟢 | 🟢 | ⭕ | ⭕ |
| 45 | listInClassCanBeReadCorrectly | 🟢 | 🟢 | ⭕ | ⭕ |
| 46 | useNestedStruct | 🟢 | [🔴](https://github.com/aws/jsii/pull/2650) | ⭕ | ⭕ |
| 46 | useNestedStruct | 🟢 | 🟢 | ⭕ | ⭕ |
| 47 | testFluentApiWithDerivedClasses | 🟢 | 🟢 | ⭕ | ⭕ |
| 48 | interfacesCanBeUsedTransparently_WhenAddedToJsiiType | 🟢 | 🟢 | ⭕ | ⭕ |
| 49 | canOverrideProtectedGetter | 🟢 | [🔴](https://github.com/aws/jsii/issues/2673) | ⭕ | ⭕ |
Expand All @@ -69,7 +69,7 @@ This section details the current state of each language binding with respect to
| 60 | canOverrideProtectedSetter | 🟢 | [🔴](https://github.com/aws/jsii/issues/2673) | ⭕ | ⭕ |
| 61 | asyncOverrides_callAsyncMethod | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) | ⭕ | ⭕ |
| 62 | nodeStandardLibrary | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) | ⭕ | ⭕ |
| 63 | dates | 🟢 | [🔴](https://github.com/aws/jsii/issues/2659) | ⭕ | ⭕ |
| 63 | dates | 🟢 | 🟢 | ⭕ | ⭕ |
| 64 | collectionOfInterfaces_ListOfStructs | 🟢 | 🟢 | ⭕ | ⭕ |
| 65 | objRefsAreLabelledUsingWithTheMostCorrectType | 🟢 | 🔴 | ⭕ | ⭕ |
| 66 | unionPropertiesWithBuilder | 🟢 | 🟢 | ⭕ | ⭕ |
Expand All @@ -86,7 +86,7 @@ This section details the current state of each language binding with respect to
| 77 | asyncOverrides_overrideThrows | 🟢 | [🔴](https://github.com/aws/jsii/issues/2670) | ⭕ | ⭕ |
| 78 | callMethods | 🟢 | 🟢 | ⭕ | ⭕ |
| 79 | returnAbstract | 🟢 | 🟢 | ⭕ | ⭕ |
| 80 | dynamicTypes | 🟢 | [🔴](https://github.com/aws/jsii/issues/2659) | ⭕ | ⭕ |
| 80 | dynamicTypes | 🟢 | 🟢 | ⭕ | ⭕ |
| 81 | hashCodeIsResistantToPropertyShadowingResultVariable | 🟢 | ⚪ | ⭕ | ⭕ |
| 82 | returnSubclassThatImplementsInterface976 | 🟢 | 🟢 | ⭕ | ⭕ |
| 83 | structs_optionalEquals | 🟢 | ⚪ | ⭕ | ⭕ |
Expand Down
39 changes: 15 additions & 24 deletions packages/@jsii/go-runtime-test/project/compliance_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,21 +62,16 @@ func (suite *ComplianceSuite) TestPrimitiveTypes() {
types.SetNumberProperty(jsii.Number(1234))
assert.Equal(float64(1234), *types.NumberProperty())

// // json
// json
mapProp := map[string]interface{}{"Foo": map[string]interface{}{"Bar": 123}}
types.SetJsonProperty(&mapProp)
assert.Equal(float64(123), (*types.JsonProperty())["Foo"].(map[string]interface{})["Bar"])

suite.FailTest("Dates are currently treated as strings and fail going through the wire", "https://github.com/aws/jsii/issues/2659")

// whoops - should accept time.Time, not string.
// date
types.SetDateProperty(jsii.String("12345"))
assert.Equal("12345", types.DateProperty())
types.SetDateProperty(jsii.Time(time.Unix(0, 123000000)))
assert.WithinDuration(time.Unix(0, 123000000), *types.DateProperty(), 0)
}

func (suite *ComplianceSuite) TestUseNestedStruct() {
suite.FailTest("Nested types are not namespaced", "https://github.com/aws/jsii/pull/2650")
jcb.StaticConsumer_Consume(customsubmodulename.NestingClass_NestedStruct{
Name: jsii.String("Bond, James Bond"),
})
Expand Down Expand Up @@ -128,15 +123,14 @@ func (suite *ComplianceSuite) TestMaps() {

func (suite *ComplianceSuite) TestDates() {
assert := suite.Assert()
suite.FailTest("Dates are represented as strings instead of date objects", "https://github.com/aws/jsii/issues/2659")

types := calc.NewAllTypes()
//types.SetDateProperty(time.Unix(128, 0))
assert.Equal(time.Unix(128, 0), types.DateProperty())
types.SetDateProperty(jsii.Time(time.Unix(128, 0)))
assert.WithinDuration(time.Unix(128, 0), *types.DateProperty(), 0)

// weak type
types.SetAnyProperty(time.Unix(999, 0))
assert.Equal(time.Unix(999, 0), types.AnyProperty())
assert.WithinDuration(time.Unix(999, 0), types.AnyProperty().(time.Time), 0)
}

func (suite *ComplianceSuite) TestCallMethods() {
Expand Down Expand Up @@ -226,9 +220,7 @@ func (suite *ComplianceSuite) TestDynamicTypes() {

// date
types.SetAnyProperty(time.Unix(1234, 0))
if types.AnyProperty() != time.Unix(1234, 0) {
suite.FailTest("Dates not supported", "https://github.com/aws/jsii/issues/2659")
}
assert.WithinDuration(time.Unix(1234, 0), types.AnyProperty().(time.Time), 0)
}

func (suite *ComplianceSuite) TestArrayReturnedByMethodCanBeRead() {
Expand Down Expand Up @@ -1175,7 +1167,6 @@ func (suite *ComplianceSuite) TestUndefinedAndNull() {

func (suite *ComplianceSuite) TestStructs_SerializeToJsii() {
t := suite.T()
t.Skip("DateTime fields are not implemented yet")

firstStruct := calclib.MyFirstStruct{
Astring: jsii.String("FirstString"),
Expand All @@ -1186,12 +1177,12 @@ func (suite *ComplianceSuite) TestStructs_SerializeToJsii() {
doubleTrouble := calc.NewDoubleTrouble()

derivedStruct := calc.DerivedStruct{
NonPrimitive: doubleTrouble,
Bool: jsii.Bool(false),
// TODO: AnotherRequired: time.Now(),
Astring: jsii.String("String"),
Anumber: jsii.Number(1234),
FirstOptional: &[]*string{jsii.String("one"), jsii.String("two")},
NonPrimitive: doubleTrouble,
Bool: jsii.Bool(false),
AnotherRequired: jsii.Time(time.Now()),
Astring: jsii.String("String"),
Anumber: jsii.Number(1234),
FirstOptional: &[]*string{jsii.String("one"), jsii.String("two")},
}

gms := calc.NewGiveMeStructs()
Expand Down Expand Up @@ -1334,8 +1325,8 @@ func NewPartiallyInitializedThisConsumerImpl(assert *assert.Assertions) *partial
return &p
}

func (p *partiallyInitializedThisConsumerImpl) ConsumePartiallyInitializedThis(obj calc.ConstructorPassesThisOut, dt *string, ev calc.AllTypesEnum) *string {
epoch := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC).Format("2006-01-02T15:04:05.000Z07:00")
func (p *partiallyInitializedThisConsumerImpl) ConsumePartiallyInitializedThis(obj calc.ConstructorPassesThisOut, dt *time.Time, ev calc.AllTypesEnum) *string {
epoch := time.Date(1970, time.January, 1, 0, 0, 0, 0, time.UTC)

p.assert.NotNil(obj)
p.assert.Equal(epoch, *dt)
Expand Down
4 changes: 4 additions & 0 deletions packages/@jsii/go-runtime/jsii-runtime-go/internal/api/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,10 @@ type EnumRef struct {
MemberFQN string `json:"$jsii.enum"`
}

type WireDate struct {
Timestamp string `json:"$jsii.date"`
}

type WireMap struct {
MapData map[string]interface{} `json:"$jsii.map"`
}
Expand Down
31 changes: 28 additions & 3 deletions packages/@jsii/go-runtime/jsii-runtime-go/internal/kernel/conversions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package kernel

import (
"reflect"
"time"

"github.com/aws/jsii-runtime-go/internal/api"
)
Expand Down Expand Up @@ -126,6 +127,11 @@ func (c *Client) CastPtrToRef(dataVal reflect.Value) interface{} {
return nil
}

// In case we got a time.Time value (or pointer to one).
if wireDate, isDate := castPtrToDate(dataVal); isDate {
return wireDate
}

switch dataVal.Kind() {
case reflect.Map:
result := api.WireMap{MapData: make(map[string]interface{})}
Expand Down Expand Up @@ -192,6 +198,24 @@ func (c *Client) CastPtrToRef(dataVal reflect.Value) interface{} {
return dataVal.Interface()
}

// castPtrToDate obtains an api.WireDate from the provided reflect.Value if it
// represents a time.Time or *time.Time value. It accepts both a pointer and
// direct value as a convenience (when passing time.Time through an interface{}
// parameter, having to unwrap it as a pointer is annoying and unneeded).
func castPtrToDate(data reflect.Value) (wireDate api.WireDate, ok bool) {
var timestamp *time.Time
if timestamp, ok = data.Interface().(*time.Time); !ok {
var val time.Time
if val, ok = data.Interface().(time.Time); ok {
timestamp = &val
}
}
if ok {
wireDate.Timestamp = timestamp.Format(time.RFC3339Nano)
}
return
}

func castValToRef(data reflect.Value) (ref api.ObjectRef, ok bool) {
if data.Kind() == reflect.Map {
for _, k := range data.MapKeys() {
Expand Down Expand Up @@ -234,13 +258,14 @@ func castValToRef(data reflect.Value) (ref api.ObjectRef, ok bool) {
}

// TODO: This should return a time.Time instead
func castValToDate(data reflect.Value) (date string, ok bool) {
func castValToDate(data reflect.Value) (date time.Time, ok bool) {
if data.Kind() == reflect.Map {
for _, k := range data.MapKeys() {
v := reflect.ValueOf(data.MapIndex(k).Interface())
if k.Kind() == reflect.String && k.String() == "$jsii.date" && v.Kind() == reflect.String {
date = v.String()
ok = true
var err error
date, err = time.Parse(time.RFC3339Nano, v.String())
ok = (err == nil)
break
}
}
Expand Down
16 changes: 12 additions & 4 deletions packages/@jsii/go-runtime/jsii-runtime-go/runtime.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"reflect"
"strings"
"time"

"github.com/aws/jsii-runtime-go/internal/api"
"github.com/aws/jsii-runtime-go/internal/kernel"
Expand Down Expand Up @@ -371,7 +372,14 @@ func Close() {
kernel.CloseClient()
}

// Helpers to store primitives and return pointers to them
func Bool(v bool) *bool { return &v }
func Number(v float64) *float64 { return &v }
func String(v string) *string { return &v }
// Bool obtains a pointer to the provided bool.
func Bool(v bool) *bool { return &v }

// Number obtains a pointer to the provided float64.
func Number(v float64) *float64 { return &v }

// String obtains a pointer to the provided string.
func String(v string) *string { return &v }

// Time obtains a pointer to the provided time.Time.
func Time(v time.Time) *time.Time { return &v }
17 changes: 17 additions & 0 deletions packages/jsii-pacmak/lib/targets/go/dependencies.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* Information about a module's dependency on "special" packages (either part of
* the go standard library, or generated as part of the current module).
*/
export interface SpecialDependencies {
/** Whether the jsii runtime library for go is needed */
readonly runtime: boolean;

/** Whether the package's initialization hook is needed */
readonly init: boolean;

/** Whether the internal type aliases package is needed */
readonly internal: boolean;

/** Whether go's standard library "time" module is needed */
readonly time: boolean;
}
39 changes: 25 additions & 14 deletions packages/jsii-pacmak/lib/targets/go/package.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { basename, dirname, join } from 'path';
import * as semver from 'semver';

import { VERSION } from '../../version';
import { SpecialDependencies } from './dependencies';
import { EmitContext } from './emit-context';
import { ReadmeFile } from './readme-file';
import {
Expand Down Expand Up @@ -184,16 +185,18 @@ export abstract class Package {
code.line();
}

protected get usesRuntimePackage(): boolean {
return this.types.some((type) => type.usesRuntimePackage);
}

protected get usesInitPackage(): boolean {
return this.types.some((type) => type.usesInitPackage);
}

protected get usesInternalPackage(): boolean {
return this.types.some((type) => type.usesInternalPackage);
protected get specialDependencies(): SpecialDependencies {
return this.types
.map((t) => t.specialDependencies)
.reduce(
(acc, elt) => ({
runtime: acc.runtime || elt.runtime,
init: acc.init || elt.init,
internal: acc.internal || elt.internal,
time: acc.time || elt.time,
}),
{ runtime: false, init: false, internal: false, time: false },
);
}

/**
Expand Down Expand Up @@ -229,18 +232,24 @@ export abstract class Package {
private emitImports(code: CodeMaker) {
const toImport = new Array<ImportedModule>();

if (this.usesRuntimePackage) {
const specialDeps = this.specialDependencies;

if (specialDeps.time) {
toImport.push({ module: 'time' });
}

if (specialDeps.runtime) {
toImport.push(JSII_RT_MODULE);
}

if (this.usesInitPackage) {
if (specialDeps.init) {
toImport.push({
alias: JSII_INIT_ALIAS,
module: `${this.root.goModuleName}/${JSII_INIT_PACKAGE}`,
});
}

if (this.usesInternalPackage) {
if (specialDeps.internal) {
toImport.push({
module: `${this.goModuleName}/${INTERNAL_PACKAGE_NAME}`,
});
Expand Down Expand Up @@ -611,7 +620,9 @@ function importGoModules(code: CodeMaker, modules: readonly ImportedModule[]) {
}

function isBuiltIn(mod: ImportedModule): boolean {
return !mod.module.includes('/');
// Standard library modules don't have any "." in their path, whereas any
// other module has a DNS portion in them, which must include a ".".
return !mod.module.includes('.');
}

function isSpecial(mod: ImportedModule): boolean {
Expand Down
Loading

0 comments on commit 2ba2ec4

Please sign in to comment.