Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(clientapi): Fix generated TypeScript admin client code to correspond to test data #1478

Merged
merged 6 commits into from Oct 23, 2019

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Oct 17, 2019

Fixes #1476.

@tobiasschweizer
Copy link
Contributor

I got into some issues because properties are now declared as optional:

@JsonObject("UsersResponse")
export class UsersResponse {

    /**
     * The users returned in a UsersResponse.
     */
    @JsonProperty("users", [ReadUser])
    users?: ReadUser[] = [];

}
@JsonObject("GroupsResponse")
export class GroupsResponse {

    /**
     * A collection of groups.
     */
    @JsonProperty("groups", [ReadGroup])
    groups?: ReadGroup[] = [];

}

Is this intended?

@benjamingeer
Copy link
Author

I changed the TypeScript class generation template so that if a property has cardinality 0-n, it is optional. I did this in response to your problem with the members property in ReadProject. Some API operations return a project with its members, and other API operations return a project without its members. But the cardinality of members is 0-n, so the value of members could always be an empty array, and I think that in that case, it would be reasonable for the client library to also accept that the property may be absent. The easiest way to do this in TypeScript seems to be to make the property optional, and the easiest way to do that is to make all 0-n properties optional. What sort of problem does this cause? Do you see a better solution?

@tobiasschweizer
Copy link
Contributor

It's fine if the users array is empty, but it shouldn't be optional.

knoraApiConnection.admin.usersEndpoint.getUsers().subscribe(
                (response: ApiResponseData<UsersResponse>) => {
                    expect(response.body.users.length).toEqual(18);
                    expect(response.body.users[0].familyName).toEqual("Admin-alt");

                    done();
                });

It's ok if the users array is empty (length zero), but this code breaks if users is undefined. If users is optional, this would require a check if it is undefined each time before accessing it. This is why an empty array is preferable.

@benjamingeer
Copy link
Author

OK then I guess I have to make a ProjectBasicInfo class without members, with a ProjectMembershipsResponse class to wrap it in.

@tobiasschweizer
Copy link
Contributor

OK then I guess I have to make a ProjectBasicInfo class without members, with a ProjectMembershipsResponse class to wrap it in.

If that is possible, this would be great!

I am just writing a short guideline how to integrate and try the generated code, will be ready soon.

@subotic
Copy link
Collaborator

subotic commented Oct 17, 2019

I thought that the admin API will always return an empty array if the sequence is empty. Are there any routes that don't behave like this?

@benjamingeer
Copy link
Author

@subotic The route /admin/users/iri/USER_IRI/project-memberships returns a response like this:

{
   "projects":[
      {
         "description":[
            {
               "value":"Anything Project"
            }
         ],
         "id":"http://rdfh.ch/projects/0001",
         "keywords":[

         ],
         "logo":null,
         "longname":"Anything Project",
         "ontologies":[
            "http://www.knora.org/ontology/0001/anything",
            "http://www.knora.org/ontology/0001/something"
         ],
         "selfjoin":false,
         "shortcode":"0001",
         "shortname":"anything",
         "status":true
      }
   ]
}

In other admin API responses, a project has a members property (with cardinality 0-n), but in this response, it doesn't have that property.

@subotic
Copy link
Collaborator

subotic commented Oct 17, 2019

The ProjectADM case class doesn't have a members property at all, and this is what is returned here.

Which routes return the members property together with a project? Strange, I wouldn't know how that could be possible.

@benjamingeer
Copy link
Author

@subotic You're right, this must be my mistake. Somehow I must have got confused and thought that the project class needed a members property in some cases. That makes it easier to fix this bug. :)

@tobiasschweizer
Copy link
Contributor

@benjamingeer I suggest to combine the review of this PR with the review of dasch-swiss/dsp-js-lib#72. dasch-swiss/dsp-js-lib#72 should be ready soon.

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again!

@benjamingeer
Copy link
Author

@tobiasschweizer Thanks for your help and patience with this!

@tobiasschweizer
Copy link
Contributor

@benjamingeer please coordinate with @subotic for merging, he is preparing a new release.

@tobiasschweizer
Copy link
Contributor

Thanks for your help and patience with this!

Thank you :-)

@benjamingeer
Copy link
Author

@subotic Should I merge this today, or wait till next month? It's not a breaking change and only affects client code generation.

@benjamingeer benjamingeer merged commit f089f4b into develop Oct 23, 2019
@benjamingeer benjamingeer deleted the fix/1476-admin-client branch October 23, 2019 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin client code generation
3 participants