Azure-sdk-for-go: [datalake-store-filesystem] JSON Unmarshal for ACLStatus fails

[afsalthaj]

The JSON Unmarshal for ACLStatus fails giving wrong results when calling GetACLStatus.

Fix: Change the type of Permission from int32 to string

type ACLStatus struct {
    Entries    *[]string `json:"entries,omitempty"`
    Group      *string   `json:"group,omitempty"`
    Owner      *string   `json:"owner,omitempty"`
    Permission *string   `json:"permission,omitempty"`
    StickyBit  *bool     `json:"stickyBit,omitempty"`
}

[joshgav]

Thanks @afsalthaj. Can you share the line of code which leads to this error? Thanks.

This also looks like a spec bug here: https://github.com/Azure/azure-rest-api-specs/blob/current/specification/datalake-store/data-plane/Microsoft.DataLakeStore/2016-11-01/filesystem.json#L1492

cc @begoldsm

[afsalthaj]

@joshgav

var tooId bool = false //get from somewhere else

    acl, err := datalakeFsClient.GetACLStatus(accountName, pathParameter, "GETACLSTATUS", &tooId)

Thanks

[Meanwhile, I have raised an issue on avoid passing operation constants (ex: GETACLSTATUS) from user side, #905]

[begoldsm]

Unless something has changed, the permission that is passed to the service should continue to be an octal number. We want to explicitly use an octal number type, but that doesn't necessarily exist across all languages, so we settled on int32 + documentation. Is int32 not a supported format for go?

[marstr]

Interesting, I don't think this is a spec-bug. We should go ensure that autorest.go is doing the right thing.

[joshgav]

@marstr what do you think the issue is? thanks!

[jhendrixMSFT]

@begoldsm Go supports int32. With the amount of info present I would guess that the service is returning the permissions number as a string instead of an int. I've been trying to repro this but I haven't been able to figure out how to configure my service principal so that it can authenticate with data lake (see below error).

{"RemoteException":{"exception":"AccessControlException","message":"GETACLSTATUS failed with error 0x83090aa2 (Forbidden. ACL verification failed. Either the resource does not exist or the user is not authorized to perform the requested operation.). [935190f5-bdaf-4b70-afce-213164c076e1] failed with error 0x83090aa2 (Forbidden. ACL verification failed. Either the resource does not exist or the user is not authorized to perform the requested operation.). [935190f5-bdaf-4b70-afce-213164c076e1][2018-01-03T14:57:45.2529074-08:00]","javaClassName":"org.apache.hadoop.security.AccessControlException"}}

[thsutton]

Here's the output that needs to be parsed:

$ az dls fs access show --account mydatalakestoreaccount --path /
{
  "entries": [
    "user::rwx",
    "group::rwx",
    "mask::rwx",
    "other::---",
    "default:user::rwx",
    "default:group::rwx",
    "default:mask::rwx",
    "default:other::---"
  ],
  "group": "uuuuuuuu-uuuu-iiii-iiii-dddddddddddd",
  "owner": "uuuuuuuu-uuuu-iiii-iiii-dddddddddddd",
  "permission": "770",
  "stickyBit": false
}

[jhendrixMSFT]

@thsutton Thanks! So indeed permission is coming across the wire as a string not an int.
@begoldsm This is a bug in the swagger, https://github.com/Azure/azure-rest-api-specs/blob/master/specification/datalake-store/data-plane/Microsoft.DataLakeStore/stable/2016-11-01/filesystem.json#L1491 states that the type is integer, it should be string (I assume that changing the format sent over the wire is not an option).

[jhendrixMSFT]

Hello @begoldsm have you had a chance to review my assessment of the issue?

[begoldsm]

@jhendrixMSFT I both agree and disagree. For all other language parsers they can correctly turn this "string" value into an integer, which is what was asked of the client tools for this value. Changing the value in swagger would be a breaking change for all other SDKs in all other languages, which is pretty heavy. Is it difficult for the go json parser to do parseInt() or the equivalent for this object? I am fairly certain that is what all other language generators are doing.

Also, for the future please engage with @ro-joowan for these issues, as I am no longer actively working on ADL (just lurking 😄 )

[jhendrixMSFT]

@amarzavery what is your opinion here? IMO it's unfortunate that other generators paper over such a defect; this is much more difficult to do in Go as the default unmarshaler expects the types to exactly match (emitting a custom unmarshaler would be tricky, how would we know for this case that we need to do so?).

[amarzavery]

I think this is a spec bug. The node sdk also expects a number. This will work for deserialization because the sdk is loose w.r.t type validation during deserialization (expect the server to do the right thing). However, this will not work during serialization as the type validation will fail.

IMHO, this should be modeled in the swagger spec as a "type": "string" and we should add support for "format": "octal". The swagger specification says that "format" is free form and additional values can be added. We already support "base64url" today (which is not a specified format in swagger 2.0 specification).

We should not rely on C# behavior because newtonsoft.json is too relaxed and tries to be too intelligent at times to predict what could be the actual value.

I think it would be better to make a breaking change and do the right thing i.e. support octal numbers.

[fearthecowboy]

I think this is a spec bug

:+1: for @amarzavery

This is absolutely a spec bug. If the content is a string, then we should absolutely not rely on lax behavior. Not all customers use AutoRest generated interfaces to talk to our services, and there is absolutely no benefit to having an incorrect OpenAPI spec and then relying on the deserializer to fudge the books.

If you want to prevent a breaking change in the other SDKs, there are ways to work around that.

[jhendrixMSFT]

@amarzavery @fearthecowboy Thanks guys I've opened a specs PR to address this.

[begoldsm]

@fearthecowboy @amarzavery and @jhendrixMSFT ack! You are all right, of course. I thought for a second that it might be that there was a change on the server side, and this was initially returning a number. However, looking at past recordings, it has always been returning a string (at least since August of last year for C#: https://github.com/Azure/azure-sdk-for-net/blob/psSdkJson6/src/SDKs/DataLake.Store/DataLakeStore.Tests/SessionRecords/DataLakeStore.Tests.FileSystemOperationTests/DataLakeStoreFileSystemAppendToFile.json#L877).

Good find! The ADL team will also need to make some changes to CLI and PowerShell to ensure that correcting the spec bug doesn't cause breaks in those commands as well as create a new major version of the C# and Java SDKs.

[begoldsm]

@amarzavery I like your idea as well. Do you have an issue tracking the additional customization for octal support? That would be ideal 🍻

[jhendrixMSFT]

I've posted a PR to the specs repo, see https://github.com/Azure/azure-rest-api-specs/pull/2263.

[amarzavery]

Merged!

Thanks,
Amar Zavery

---

From: Joel Hendrix [e-mail]
Sent: Friday, January 12, 2018 12:45:12 PM
To: Azure/azure-sdk-for-go
Cc: Amar Zavery; Mention
Subject: Re: [Azure/azure-sdk-for-go] [datalake-store-filesystem] JSON Unmarshal for ACLStatus fails (#902)

I've posted a PR to the specs repo, see Azure/azure-rest-api-specs#2263https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fazure-rest-api-specs%2Fpull%2F2263&data=02%7C01%7Camzavery%40microsoft.com%7C3a25129e2bc5461e41cf08d559fd6120%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636513867152768141&sdata=4jLYiTAH4yLqO7nhm%2BX0kSQ6eavzxsvbD6BF5%2FsrocI%3D&reserved=0.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fazure-sdk-for-go%2Fissues%2F902%23issuecomment-357342636&data=02%7C01%7Camzavery%40microsoft.com%7C3a25129e2bc5461e41cf08d559fd6120%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636513867152768141&sdata=pmzGfbls04xKGgCCWXVbQxR1y01E2hZK0%2FFn284mU38%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAET9Bw6THGkt7nqFRjRpNfQdHu125JD_ks5tJ8D4gaJpZM4RB5eT&data=02%7C01%7Camzavery%40microsoft.com%7C3a25129e2bc5461e41cf08d559fd6120%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636513867152768141&sdata=Vh9EnSDY%2BE1x2qDWaSUGasSUiS1idZKnHt%2Fl3dJbt%2BA%3D&reserved=0.

[jhendrixMSFT]

Fixed in v14.0.0.