OpenSILEX / opensilexClientToolsPython

1 stars 0 forks source link

While generating group_creation_dto-objects, nested objects are not converted to models #4

Closed Dryocopus closed 3 years ago

Dryocopus commented 3 years ago

I have a json-file with a group definition like this:

[
  { 
    "uri": "http://opensilex.dev/groups#Experiment_manager2",
    "rdf_type": "os-sec:Group",
    "rdf_type_name": "group",
    "name": "Experiment manager VTC-2021a",
    "description": "Group for all experiments managers",
    "user_profiles": [
      {
        "uri": "",
        "rdf_type": "os-sec:GroupUserProfile",
        "rdf_type_name": "User profile",
        "profile_uri": "uri_for(profile, Data-admin)",
        "profile_name": "", 
        "user_uri": "uri_for(user, peter.roos)",
        "user_name": "" 
      },
      {
        "uri": "",
        "rdf_type": "os-sec:GroupUserProfile",
        "rdf_type_name": "User profile",
        "profile_uri": "uri_for(profile, data-admin)",
        "profile_name": "",
        "user_uri": "uri_for(user, peter.roos)",
        "user_name": ""
      }
    ]
  }
]

The ‘uri_for()’ entries are not relevant here, these are instructions for my own application. I use them to retrieve the applicable uris at a later stage.

I want to convert this to a list of models for the API this way:

          with open(file_descriptor.directory_file_name) as json_file:
                group_creation_dto_list_as_dict = json.load(json_file)
                for group_creation_dto_as_dict in group_creation_dto_list_as_dict:
                    group_creation_dto = opensilexClientToolsPython.models.GroupCreationDTO(**group_creation_dto_as_dict)
                    group_creation_dto_list.append(group_creation_dto)
            return group_creation_dto_list

This works well for the group_creation_dto, but apparently the nested user_profiles are not converted to group_user_profile_dto-objects but left as dict. I can work around that by doing that conversion separately:

         with open(file_descriptor.directory_file_name) as json_file:
                group_creation_dto_list_as_dict = json.load(json_file)
                for group_creation_dto_as_dict in group_creation_dto_list_as_dict:
                    group_creation_dto = opensilexClientToolsPython.models.GroupCreationDTO(**group_creation_dto_as_dict)
                    group_user_profile_dto_list = []
                    for user_profile_as_dict in group_creation_dto.user_profiles:
                        group_user_profile_dto = opensilexClientToolsPython.models.GroupUserProfileDTO(**user_profile_as_dict)
                        group_user_profile_dto_list.append(group_user_profile_dto)
                    group_creation_dto.user_profiles = group_user_profile_dto_list
                    group_creation_dto_list.append(group_creation_dto)
            return group_creation_dto_list

But in my opinion, that’s not ideal from a conceptual point of view. The constructor of the upper-level object should trigger conversion to models for lower level objects as well.

Note: The uri_for()-entries mentioned above are still there at the movement of conversion to dto-objects, but I don’t think they cause the problem, because then the conversion in this line: group_user_profile_dto = opensilexClientToolsPython.models.GroupUserProfileDTO(**user_profile_as_dict) wouldn’t work either.

Isabelle-inrae commented 3 years ago

Hi Peter, if I understand your feedback correctly, you would like the elements included in the objects to be automatically deserialized?

This is indeed a design issue, but as the package is automatically generated and matches the Swagger template, we don't know how to work around it. Do you have any ideas/suggestions? I saw that you managed to work around the problem by adding several lines of code, could you tell us if this problem seems to you to be annoying and a priority or if it is an anomaly that can be worked around without causing too much discomfort?

Thanks in advance for your feedback,

Isabelle

Dryocopus commented 3 years ago

Hi Isabelle,

Yes, ideally. That's one of the reasons why I wrote the logic myself for the earlier version of PHIS. For this version, I didn't consider that doable given the amount of models.

But I can see that if you generate the logic with Swagger there is no way to fix it. And it isn't really an issue, because these objects aren't nested very deeply.

So then you can ignore the suggestion.

Peter

Isabelle-inrae commented 3 years ago

Ok, thank you for your feedback Peter.

Dryocopus commented 3 years ago

Hi,

I can work with the current setup, so I'll close the issue, but there might actually be a way to solve the issue.

For another external API, I generated an API-client with Swagger myself. That has model classes that are nested very deeply and they seem to be generated recursively just like I suggested above. Not sure what's the difference. Should you want to fix the issue at some later stage, I can probably look into that. But I'll concentrate on PHIS first.

Thanks for your input.

Isabelle-inrae commented 3 years ago

Hello Peter, Ok thank you, that's good to know and this could be a solution to explore ! Could you please tell us :

Thanks again for your precious feedbacks, Isabelle

Dryocopus commented 3 years ago

Hi Isabelle,

Swagger/OpenAPI version was 2, just as the PHIS-spec. Then I just used the online option Generate Client > Python:

Capture

But I must be careful with what I'm saying because for PHIS I'm currently mostly uploading stuff, while for the other system I used the generated logic mostly for downloading. That might affect the way the model classes are generated on the background.

So it is possible that I just didn't encounter the issue there because the few uploads I did weren't nested deeply or because my own logic simply worked a bit differently. Not sure - this I why I said '...seem to be generated...' above.

Or, also possible, the other party used a different style in their way of specifying the Swagger-definition (there was a huge amount of square brackets in their models).

It might be time-consuming to get into the bottom of this, and because, as said, I can work with the current setup, it is probably not worth the effort for now, unless there are also other users of PHIS who may potentially want this.

Isabelle-inrae commented 3 years ago

Ok, thanks for all these informations. Indeed for the moment we will leave this subject aside, we will let you know if we ever take it on !