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

Define openapi annotations for ChangeDetection Configuration, remove undefined keys: Fixes #1380 #1388

Closed
wants to merge 1 commit into from

Conversation

johnaohara
Copy link
Member

Fixes Issue

Fixes #1380

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@johnaohara johnaohara self-assigned this Feb 23, 2024
@johnaohara
Copy link
Member Author

@willr3 please can you take a look at this PR?

I am not happy with https://github.com/Hyperfoil/Horreum/pull/1388/files#diff-a2f50f09cbcbc88a71fe29ebafd1d19184ef91c31c39a3729dbc1f3e8444cdbeR273
or
https://github.com/Hyperfoil/Horreum/pull/1388/files#diff-a2f50f09cbcbc88a71fe29ebafd1d19184ef91c31c39a3729dbc1f3e8444cdbeR291

The problem is the way the auto-generated Typescript is de-serializing and type checking the Change Detection Config Objects.

For RelativeDifferenceOfMeans, the config object has the following structure;

{
  "filter": "mean", 
  "window": 1, 
  "threshold": 0.2, 
  "minPrevious": 5
}

However, the Change Detection Configuration is de-serialized by (auto-generated);

export function ChangeDetectionConfigFromJSONTyped(json: any, ignoreDiscriminator: boolean): ChangeDetectionConfig {
    if ((json === undefined) || (json === null)) {
        return json;
    }
    return { ...FixedThresholdDetectionFromJSONTyped(json, true), ...RelativeDifferenceDetectionFromJSONTyped(json, true) };
}

and FixedThresholdDetectionFromJSONTyped(json, true) is (auto-generated);

export function FixedThresholdDetectionFromJSONTyped(json: any, ignoreDiscriminator: boolean): FixedThresholdDetection {
    if ((json === undefined) || (json === null)) {
        return json;
    }
    return {
        
        'min': FixThresholdFromJSON(json['min']),
        'max': FixThresholdFromJSON(json['max']),
    };
}

Which returns the following if min/max is not set;

{
  "min": undefined, 
  "max": undefined
}

The resulting configuration object for is a combination of ALL the keys from ALL the configuration types;

{
  "filter": "mean", 
  "window": 1, 
  "threshold": 0.2, 
  "minPrevious": 5,
  "min": undefined, 
  "max": undefined
}

Now... when the object is serialized to send back to the server, the first check is what Type the object is, for Fix Threshold this is the check (auto-generated):

export function instanceOfFixedThresholdDetection(value: object): boolean {
    let isInstance = true;
    isInstance = isInstance && "min" in value;
    isInstance = isInstance && "max" in value;

    return isInstance;
}

Which just checks to see if the object has the min and max fields, which ofc it does, but they are undefined. So the call back to the server uses FixedThresholdDetection to Serialize a RelativeDifferenceDetection object

I have not had time to investigate is the serialization or instanceOf code can be overwritten, I have just removed the undefined keys from the object and everything "works" (until the API return types change and it will break again)

We need a better solution, but I have run out of time :(

@johnaohara
Copy link
Member Author

urgh... is failing CI, works locally :(

@johnaohara johnaohara closed this Feb 23, 2024
@johnaohara
Copy link
Member Author

Am closing this as we need a proper solution, but leaving it for reference

@johnaohara
Copy link
Member Author

The openapi changes are still valid though!

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.

Unable to modify configuration of change detection models
1 participant