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

Review connection id data type and allowed values on swagger #257

Closed
italovalcy opened this issue Apr 12, 2024 · 2 comments · Fixed by #225
Closed

Review connection id data type and allowed values on swagger #257

italovalcy opened this issue Apr 12, 2024 · 2 comments · Fixed by #225
Assignees
Labels
bug Something isn't working prio_high

Comments

@italovalcy
Copy link
Contributor

Hi,

Currently the connection ID is specified to be an Integer ranging from 1 to 10, which does not sounds reasonable. I think we should support creation of more than 10 "connections". Any reason to limit the number of connections?

https://github.com/atlanticwave-sdx/sdx-controller/blob/main/sdx_controller/swagger/swagger.yaml#L175-L176

Additionally maybe the connection ID should be generated by the controller, not provided by the user! Specially because the connection id is being used to persist data on database. I think we shouldn't rely on the user input to guarantee uniqueness of important fields like this.

What do you guys think?

@sajith
Copy link
Member

sajith commented Apr 12, 2024

@italovalcy I too don't think integers make a good connection IDs, because integers do not meet uniqueness properties we require. I also agree that the controller should generate connection IDs.

That is what I'm attempting in PR #225: POST /connection handler will generate a UUID to represent connection ID, and GET /connection/:connection_id and then DELETE /connection/:connection_id should use this generated ID. There are a couple of related issues mentioned there.

@italovalcy italovalcy added bug Something isn't working prio_high labels Apr 24, 2024
@sajith sajith linked a pull request Apr 24, 2024 that will close this issue
@sajith
Copy link
Member

sajith commented Apr 24, 2024

#225 made connection IDs integers, and removed the minimum and maximum limits. I believe that addresses this.

@sajith sajith closed this as completed Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working prio_high
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants