Skip to content

Phg/dto kickoff #1249

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

Merged
merged 8 commits into from
May 23, 2025
Merged

Phg/dto kickoff #1249

merged 8 commits into from
May 23, 2025

Conversation

Pgarrett
Copy link
Collaborator

@Pgarrett Pgarrett commented May 21, 2025

What's in the box?

Kickoff for using DTOs instead of stringified json for REST request payloads.
Current limitations:

  • it only supports Java as output format language
  • supported fields do not include lists nor custom nested objects
  • no strategy yet defined for DTOs defined in a spec path (not as a component with a predefined name)
  • DTOs not instantiated for to be used in the test payload
  • DTO creation should be done before splitting tests, to avoid re-writting and to have complete DTOs

The current implementation is able to handle wanted field scenarios using optionals and jackson:

  • field is non null and sent in the payload: {"myKey": "myValue"}/{"myKey": ""}
  • field is null and sent in the payload: {"myKey": null}
  • field is not sent in the payload: {}

For example, a spec with the following components:

  schemas:
    Person:
      type: object
      properties:
        name:
          type: string
        age:
          type: integer
        address:
          $ref: "#/components/schemas/Address"
      required:
        - name
        - age
        - address
    Address:
      type: object
      properties:
        street:
          type: string
        country:
          type: string
        voted:
          type: boolean
      required:
        - street
        - country

Will generate a class such as:

package dto;

import java.util.Optional;

import com.fasterxml.jackson.annotation.JsonInclude;
import shaded.com.fasterxml.jackson.annotation.JsonProperty;

@JsonInclude(JsonInclude.Include.NON_NULL)
public class Person {

    @JsonProperty("name")
    private Optional<String> name;

    @JsonProperty("age")
    private Optional<Integer> age;

    public Optional<String> getName() {
        return name;
    }
    
    public void setName(String name) {
        this.name = Optional.ofNullable(name);
    }

    public Optional<Integer> getAge() {
        return age;
    }
    
    public void setAge(Integer age) {
        this.age = Optional.ofNullable(age);
    }

}

@Pgarrett Pgarrett requested a review from arcuri82 May 21, 2025 03:05
@arcuri82
Copy link
Collaborator

@Pgarrett

a comment regarding

field is empty and sent in the payload: {"myKey": ""}

an empty string in JSON is a valid string. i guess you should make sure to handle the case of null, eg, {"myKey": null}

dtoFilename = TestSuiteFileName(appendDtoPackage(dtoClass.name))
}

fun write() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason to create a class with constructor, instead of having an object (eg object JavaDtoWriter) with a parametric fun write(...) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, two reasons:

  1. Hadn't thought about it, I just got writing and my first impulse was to add that information in the constructor.
  2. This writer will most likely change. I was thinking this would change into some kind of delegate/decorator. You should actually ask the DtoClass to write itself, which in turn would call the right type of DtoWriter

private fun getDtos(): List<DtoClass> {
val restSampler = sampler as AbstractRestSampler
val result = mutableListOf<DtoClass>()
restSampler.getActionDefinitions().forEach { action ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

you could move the code here under some utility function in the dto package, and then call it with restSampler.getActionDefinitions() as input

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, in the future I was actually thinking of creating a new dedicated class for this, but for starters I added here in the TestSuiteWriter. The class-to-be would actually have to run before tests are split, so as to avoid re-writing or even overwriting DTOs. Then the generated DTOs should be shared with the TestCaseWriter, not sure how yet

val primaryGene = child.primaryGene()
// TODO: Payloads could also be json arrays, analyze ArrayGene
if (primaryGene is ObjectGene) {
// TODO: Determine strategy for objects that are not defined as a component and do not have a name
Copy link
Collaborator

Choose a reason for hiding this comment

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

could use something related to action.getName, and put this as a comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could be, yes. We'll most likely come back to this later on

private fun getDtoField(field: Gene): DtoField {
val wrappedGene = GeneUtils.getWrappedValueGene(field)
return DtoField(field.name, when (wrappedGene) {
// TODO: handle nested arrays, objects and extend type system for dto fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

you will need to handle the cases of OptionalGene and NullableGene (see how wrapping works), as that will impact what kinds of field types you will need to create. also RegexGene is going to be common. anyway, you will see what things crashing when we activate this feature in the E2E... :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a matter of fact, we need more E2E tests! Most e2e tests check on tests generated using GET:

  • GET: 103+ matches in 36+ files
  • POST: 67 matches in 36 files
  • PUT: 6 matches in 5 files

And I looking into the POST tests, some do not have a DTO request payload but instead a response one, or are using simple cases. Looked into that for debugging

is FloatGene -> "Float"
is Base64StringGene -> "String"
is DateGene -> "String"
is TimeGene -> "String"
Copy link
Collaborator

Choose a reason for hiding this comment

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

you might want to use special types for this, and, in the DTOs, throw an IllegalArgumentExcpetion if invalid string. however, for now can leave it as acomment (as we still create invalid strings outside robustness testing, and that is not fixed yet)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe it's something the Gene could answer by itself, something like: gene.getDtoType() and have that answer back. That would also save us from these giant ifs

One other question I have is, for these specific cases of Time and Date, should we in the future handle them with jodatime objects or similar? Or just set them as strings and keep it simple? For a start I think strings are fine, not sure what you had in mind for the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

for now we can keep it simple. then we can see feedback from test engineers at VW

@Pgarrett Pgarrett requested a review from arcuri82 May 21, 2025 13:31
is FloatGene -> "Float"
is Base64StringGene -> "String"
is DateGene -> "String"
is TimeGene -> "String"
Copy link
Collaborator

Choose a reason for hiding this comment

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

for now we can keep it simple. then we can see feedback from test engineers at VW

@arcuri82 arcuri82 merged commit f223a36 into master May 23, 2025
14 checks passed
@Pgarrett Pgarrett deleted the phg/dtoKickoff branch May 27, 2025 12:11
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.

2 participants