-
Notifications
You must be signed in to change notification settings - Fork 43
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
refactor(web): replace NetworkClient with queries #1519
Changes from all commits
89a779c
7d7041e
dd2f88a
1f97471
ced72d2
6dfd01b
1c49e5d
9c03398
935cd93
12aa549
274e7a8
35249de
509a647
7ddf4a1
574ddab
320a372
d681c48
4e506f3
1b2ec19
bc49841
afead73
09dbfc8
60b2728
a3d0c4d
6b5a0c5
f4a4445
217b934
078c7f1
f404bc9
d833469
539c23c
4889a91
b28be07
56df1f2
7b74a34
a56665c
019156b
2b6fcd5
32930e0
96d2b9b
fb4ebb9
d090cde
29f4a73
0352995
fab8a0a
f52a5ad
d588c38
02220c0
27e17bb
80d11dd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,7 @@ use axum::{ | |
extract::{Path, State}, | ||
http::StatusCode, | ||
response::{IntoResponse, Response}, | ||
routing::{delete, get, put}, | ||
routing::{delete, get, patch, post}, | ||
Json, Router, | ||
}; | ||
|
||
|
@@ -101,10 +101,10 @@ pub async fn network_service<T: Adapter + Send + Sync + 'static>( | |
.put(update_connection) | ||
.get(connection), | ||
) | ||
.route("/connections/:id/connect", get(connect)) | ||
.route("/connections/:id/disconnect", get(disconnect)) | ||
.route("/connections/:id/connect", patch(connect)) | ||
.route("/connections/:id/disconnect", patch(disconnect)) | ||
.route("/devices", get(devices)) | ||
.route("/system/apply", put(apply)) | ||
.route("/system/apply", post(apply)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is rationale for this change? for me the main difference between post and put is idempotency. So when I call apply twice will it do two actions and only the first apply do something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I correctly understood what @imobachgs told me last week, the rationale here is that we're using POST for these kind of actions, like it's the case of probing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are getting maybe too theoretical, but probe is not idempotent for me as next probe can find something else. But I think that apply is idempotent as it should not create/do something different. |
||
.route("/wifi", get(wifi_networks)) | ||
.with_state(state)) | ||
} | ||
|
@@ -293,7 +293,7 @@ async fn update_connection( | |
} | ||
|
||
#[utoipa::path( | ||
get, | ||
patch, | ||
path = "/connections/:id/connect", | ||
context_path = "/api/network", | ||
responses( | ||
|
@@ -325,7 +325,7 @@ async fn connect( | |
} | ||
|
||
#[utoipa::path( | ||
get, | ||
patch, | ||
path = "/connections/:id/disconnect", | ||
context_path = "/api/network", | ||
responses( | ||
|
@@ -357,7 +357,7 @@ async fn disconnect( | |
} | ||
|
||
#[utoipa::path( | ||
put, | ||
post, | ||
path = "/system/apply", | ||
context_path = "/api/network", | ||
responses( | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,7 @@ const get = (url: string) => http.get(url).then(({ data }) => data); | |
* @param url - endpoint URL | ||
* @param data - Request payload | ||
*/ | ||
const patch = (url: string, data: object) => http.patch(url, data); | ||
const patch = (url: string, data?: object) => http.patch(url, data); | ||
|
||
/** | ||
* Performs a PUT request with the given URL and data | ||
|
@@ -55,7 +55,7 @@ const put = (url: string, data: object) => http.put(url, data); | |
* @param url - endpoint URL | ||
* @param data - request payload | ||
*/ | ||
const post = (url: string, data: object) => http.post(url, data); | ||
const post = (url: string, data?: object) => http.post(url, data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, for both changes, does it make sense to have patch and post that does not provide data? It looks like bug for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. after seeing below usage I would say that post change makes sense...but still for patch I kind of expect instructions how to modify source and below usage is at least confusing for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which verbs use for which actions / request will be for sure a topic for discussions. I personally have to revisit all the spec for refreshing my mind about HTTP verbs and the best usage of them. But as per RFC, https://www.rfc-editor.org/rfc/rfc5789, patch does not require a payload. It's optional. As you pasted in another comment, |
||
|
||
/** | ||
* Performs a DELETE request on the given URL | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
/* | ||
* Copyright (c) [2024] SUSE LLC | ||
* | ||
* All Rights Reserved. | ||
* | ||
* This program is free software; you can redistribute it and/or modify it | ||
* under the terms of version 2 of the GNU General Public License as published | ||
* by the Free Software Foundation. | ||
* | ||
* This program is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for | ||
* more details. | ||
* | ||
* You should have received a copy of the GNU General Public License along | ||
* with this program; if not, contact SUSE LLC. | ||
* | ||
* To contact SUSE LLC about this file by physical or electronic mail, you may | ||
* find current contact information at www.suse.com. | ||
*/ | ||
|
||
import { del, get, patch, post, put } from "~/api/http"; | ||
import { APIAccessPoint, APIConnection, APIDevice, NetworkGeneralState } from "~/types/network"; | ||
|
||
/** | ||
* Returns the network configuration | ||
*/ | ||
const fetchState = (): Promise<NetworkGeneralState> => get("/api/network/state"); | ||
|
||
/** | ||
* Returns a list of known devices | ||
*/ | ||
const fetchDevices = (): Promise<APIDevice[]> => get("/api/network/devices"); | ||
|
||
/** | ||
* Returns data for given connection name | ||
*/ | ||
const fetchConnection = (name: string): Promise<APIConnection> => | ||
get(`/api/network/connections/${name}`); | ||
|
||
/** | ||
* Returns the list of known connections | ||
*/ | ||
const fetchConnections = (): Promise<APIConnection[]> => get("/api/network/connections"); | ||
|
||
/** | ||
* Returns the list of known access points | ||
*/ | ||
const fetchAccessPoints = (): Promise<APIAccessPoint[]> => get("/api/network/wifi"); | ||
|
||
/** | ||
* Adds a new connection | ||
* | ||
* @param connection - connection to be added | ||
*/ | ||
const addConnection = (connection: APIConnection) => post("/api/network/connections", connection); | ||
|
||
/** | ||
* Updates given connection | ||
* | ||
* @param connection - connection to be added | ||
*/ | ||
const updateConnection = (connection: APIConnection) => | ||
put(`/api/network/connections/${connection.id}`, connection); | ||
|
||
/** | ||
* Deletes the connection matching given name | ||
*/ | ||
const deleteConnection = (name: string) => del(`/api/network/connections/${name}`); | ||
|
||
/** | ||
* Apply network changes | ||
*/ | ||
const applyChanges = () => post("/api/network/system/apply"); | ||
|
||
/** | ||
* Performs the connect action for connection matching given name | ||
*/ | ||
const connect = (name: string) => patch(`/api/network/connections/${name}/connect`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to be honest, I do not get here why it is patch..for patch it should contain patch structure that changes connection object. https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/PATCH mentions that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As commented at #1519 (comment), you can think in these actions as the instructions you're requesting for modifying the connection state. As I said in linked comment, I still having pending to refresh the HTTP verbs/requests. But I tend to think in POST as a request for creating something or appending more data to a resource (although for the latest I'd use a PUT). Also, I expect that servers returns the resource representation or at least where to retrieve it. None of these are the case for connect and disconnect actions for which we are just requiring a change in the connection state, a request side-effect. |
||
|
||
/** | ||
* Performs the disconnect action for connection matching given name | ||
*/ | ||
const disconnect = (name: string) => patch(`/api/network/connections/${name}/disconnect`); | ||
|
||
export { | ||
fetchState, | ||
fetchDevices, | ||
fetchConnection, | ||
fetchConnections, | ||
fetchAccessPoints, | ||
applyChanges, | ||
addConnection, | ||
updateConnection, | ||
deleteConnection, | ||
connect, | ||
disconnect, | ||
}; |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see below reasoning, but I think that post is better here then patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1519 (comment)
If you don't mind, we can postpone this discussion until @teclator and @imobachgs are back. As far as I know, this was a change agreed by them. So we can merge as it is and change our mind later if needed.