apache / incubator-devlake

Apache DevLake is an open-source dev data platform to ingest, analyze, and visualize the fragmented data from DevOps tools, extracting insights for engineering excellence, developer experience, and community growth.
https://devlake.apache.org/
Apache License 2.0
2.5k stars 493 forks source link

[Bug][Blueprints] Duplicate CircleCI scopes in same connection allowed #7653

Open jensrotne opened 1 week ago

jensrotne commented 1 week ago

Search before asking

What happened

I had an issue, where I was able to input the same CircleCI sco pe identifier twice in the connections field of the blueprint. This causes the API to throw a 500 request, but the blueprint was still created and ended up in a bad state, where I had to manually delete it and try again.

screenshot

What do you expect to happen

To give me a 400 response telling me that I had a duplicate value in the connections scopes.

How to reproduce

Adding a Blueprint with the something like the following connection plugin:

{
  "pluginName": "circleci",
  "connectionId": 2,
  "scopes": [
    {
      "scopeId": "1"
    },
    {
      "scopeId": "1"
    },
    {
      "scopeId": "3"
    },
    {
      "scopeId": "4"
    }
  ]
}

This will cause the API to throw an error.

Anything else

No response

Version

v1.0.0-rc1

Are you willing to submit PR?

Code of Conduct

dosubot[bot] commented 1 week ago

To address the issue of duplicate CircleCI scope identifiers in the connections field of the blueprint and ensure a 400 response instead of a 500 error, you can implement validation both on the frontend and backend.

Frontend Validation

Add a validation step before submitting the data to check for duplicate scope identifiers. Here's an example of how you can modify the handleSubmit function:

const handleSubmit = (scopeIds: any) => {
  if (!selectedConnection) return;

  // Check for duplicate scope identifiers
  const uniqueScopeIds = new Set(scopeIds);
  if (uniqueScopeIds.size !== scopeIds.length) {
    // Handle the error, e.g., show a notification or set an error state
    console.error("Duplicate scope identifiers found");
    return;
  }

  onSubmit({
    pluginName: selectedConnection.plugin,
    connectionId: selectedConnection.id,
    scopes: scopeIds.map((scopeId: any) => ({ scopeId })),
  });
};

This code snippet adds a check for duplicate scope identifiers using a Set. If duplicates are found, it logs an error message and prevents the submission. You can replace the console.error with any error handling mechanism you prefer, such as displaying a notification to the user [1].

Backend Validation

Ensure the backend API is designed to return a 400 error for bad requests, including those with duplicate scope identifiers. Modify the PostScopeConfig function in the backend to handle this validation:

func PostScopeConfig(input *plugin.ApiResourceInput) (*plugin.ApiResourceOutput, errors.Error) {
  // Validate for duplicate scope identifiers
  scopeIds := input.Body["scopeIds"].([]interface{})
  uniqueScopeIds := make(map[interface{}]bool)
  for _, id := range scopeIds {
    if uniqueScopeIds[id] {
      return nil, errors.BadInput.New("Duplicate scope identifiers found")
    }
    uniqueScopeIds[id] = true
  }

  return dsHelper.ScopeConfigApi.Post(input)
}

This backend function checks for duplicate scope identifiers and returns a 400 error if any are found, ensuring that the client receives a clear and appropriate response [2].

Implementing these changes should prevent the creation of blueprints with duplicate scope identifiers and provide a more user-friendly error response.

To continue talking to Dosu, mention @dosu.

klesh commented 1 week ago

Sounds reasonable to me. Anyone is free to work on the issue if you are interested.