-
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
first draft of questions API in http #1091
Conversation
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.
It is looking good, although I know that quite some work still remains. I just added a comment about how to implement the stream.
About answering a question, we could implement a D-Bus signal that it is more convenient than having to listen for properties changes. Is it worth it?
use serde::{Deserialize, Serialize}; | ||
use serde_json::json; | ||
|
||
// TODO: move to lib |
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.
👍
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.
only tricky part with moving to lib is that structs for web does not live there...There are GenericQuestion struct, so after move I will need to implement some Into
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.
I still think that the Client should be 1) agnostic from the web layer and 2) live in agama-lib
. However, given that we are short on time, we could add a Trello card listing things to improve and refactor. After all, we are still learning how to organize our code.
let add_stream = proxy | ||
.receive_interfaces_added() | ||
.await? | ||
.then(|_| async move { |
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.
I do not know what your plan is here, but to simplify things a bit, you could include the question number (taken from the D-Bus path) and let the client (on the JavaScript side) retrieve the question.
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.
well, current API does not allow to retrieve single question. That is different to DBus.
Basically my idea about API is:
- list all unanswered questions at
/questions
- answer specific question according to its id with
/questions/:id/answer
- get event that list of unanswered questions changed
- and now when I am thinking about it, we need also way to provide answers file, so some API for it.
Reason why I design it this way is that common workflow is question arise, answer is provided and then another question arise. I think that having multiple unanswered question is quite rare situation, so I optimize API for that single case, but do not prevent in future to extend it e.g. to also get single question if there will be many questions.
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.
OK, I get the idea. About the answers file, we are not using it right now, so you can skip that part.
Well, as said, issue is that it is not about properties, but about object manager...which makes sense for dbus, but working with it in signals is not so easy...especially when it talks about interfaces, so we had that known race condition when you have question with multiple interfaces. So maybe having specific signal on Questions interface that will just say questions changed or two specialized like |
If adding those signals could help, I am all for it. |
|
||
/// Facade of agama_lib::questions::GenericQuestion | ||
/// For fields details see it. | ||
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)] |
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.
Why do you need a facade? If it is because of the utoipa::ToSchema
, it is fine with me. I have derived the utoipa::ToSchema
for some structs in lib and it feels wrong to me.
However, having to use a facade just because of our documentation library feels wrong too.
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.
No, it is not related to utoipa at all. It is because I do not want to break dbus code and I like more the approach with generic question and composition of question parts. Original code contain QuestionWithPassword that contain link to GenericQuestion, which I do not like much.
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.
Well, as soon as you do not break the D-Bus external API, you can refactor the internals if you wish. If you decide to keep the facade, please, write down the reason in the comment.
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.
well, I will write it to comment. In DBus we have attributes and in question is included also answer attribute. Which is not what I want to http API. I have there two parts: 1. question and 2. answer. So original one struct is split into two and the first one is used as output and the second as expected input. At least that is my idea. As said I will write it to comment
use std::{collections::HashMap, pin::Pin}; | ||
use thiserror::Error; | ||
use tokio_stream::{Stream, StreamExt}; | ||
use zbus::zvariant::ObjectPath; |
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.
You can merge this line and the next.
use serde::{Deserialize, Serialize}; | ||
use serde_json::json; | ||
|
||
// TODO: move to lib |
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.
I still think that the Client should be 1) agnostic from the web layer and 2) live in agama-lib
. However, given that we are short on time, we could add a Trello card listing things to improve and refactor. After all, we are still learning how to organize our code.
} | ||
} | ||
|
||
#[derive(Error, Debug)] |
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.
This error do not bring any value. As we already did in the software layer, you can directly use crate::error::Error.
|
||
/// Facade of agama_lib::questions::GenericQuestion | ||
/// For fields details see it. | ||
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)] |
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.
Well, as soon as you do not break the D-Bus external API, you can refactor the internals if you wish. If you decide to keep the facade, please, write down the reason in the comment.
|
||
/// Facade of agama_lib::questions::WithPassword | ||
/// For fields details see it. | ||
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)] |
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.
Same than above.
); | ||
let proxy = ObjectManagerProxy::builder(&dbus) | ||
.path(question_path) | ||
.context("Failed to create object manager path")? |
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.
To be honest, I am still not a big fan of using anyhow
in libraries. But I can leave with that :-)
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.
me neither, but it is much faster then adding each zvariant error :)
After a few months of work, it is time to merge the `architecture_2024` branch into `master`. It is still a work-in-progress, but all the efforts should be go now against that branch. ## Pull requests * #1061 * #1064 * #1073 * #1074 * #1080 * #1089 * #1091 * #1092 * #1094 * #1095 * #1099 * #1100 * #1102 * #1103 * #1112 * #1114 * #1116 * #1117 * #1119 * #1120 * #1123 * #1126 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1136 * #1139 * #1140 * #1143 * #1146 ## Other commits * 8efa41f * 9e2dec0
Prepare for releasing Agama 8. It includes the following pull requests: * #884 * #886 * #914 * #918 * #956 * #957 * #958 * #959 * #960 * #961 * #962 * #963 * #964 * #965 * #966 * #969 * #970 * #976 * #977 * #978 * #979 * #980 * #981 * #983 * #984 * #985 * #986 * #988 * #991 * #992 * #995 * #996 * #997 * #999 * #1003 * #1004 * #1006 * #1007 * #1008 * #1009 * #1010 * #1011 * #1012 * #1014 * #1015 * #1016 * #1017 * #1020 * #1022 * #1023 * #1024 * #1025 * #1027 * #1028 * #1029 * #1030 * #1031 * #1032 * #1033 * #1034 * #1035 * #1036 * #1038 * #1039 * #1041 * #1042 * #1043 * #1045 * #1046 * #1047 * #1048 * #1052 * #1054 * #1056 * #1057 * #1060 * #1061 * #1062 * #1063 * #1064 * #1066 * #1067 * #1068 * #1069 * #1071 * #1072 * #1073 * #1074 * #1075 * #1079 * #1080 * #1081 * #1082 * #1085 * #1086 * #1087 * #1088 * #1089 * #1090 * #1091 * #1092 * #1093 * #1094 * #1095 * #1096 * #1097 * #1098 * #1099 * #1100 * #1102 * #1103 * #1104 * #1105 * #1106 * #1109 * #1110 * #1111 * #1112 * #1114 * #1116 * #1117 * #1118 * #1119 * #1120 * #1121 * #1122 * #1123 * #1125 * #1126 * #1127 * #1128 * #1129 * #1130 * #1131 * #1132 * #1133 * #1134 * #1135 * #1136 * #1138 * #1139 * #1140 * #1141 * #1142 * #1143 * #1144 * #1145 * #1146 * #1147 * #1148 * #1149 * #1151 * #1152 * #1153 * #1154 * #1155 * #1156 * #1157 * #1158 * #1160 * #1161 * #1162 * #1163 * #1164 * #1165 * #1166 * #1167 * #1168 * #1169 * #1170 * #1171 * #1172 * #1173 * #1174 * #1175 * #1177 * #1178 * #1180 * #1181 * #1182 * #1183 * #1184 * #1185 * #1187 * #1188 * #1189 * #1190 * #1191 * #1192 * #1193 * #1194 * #1195 * #1196 * #1198 * #1199 * #1200 * #1201 * #1203 * #1204 * #1205 * #1206 * #1207 * #1208 * #1209 * #1210 * #1211 * #1212 * #1213 * #1214 * #1215 * #1216 * #1217 * #1219 * #1220 * #1221 * #1222 * #1223 * #1224 * #1225 * #1226 * #1227 * #1229
Problem
Questions are not exposed in new architecture 2024
Solution
Add path to enlist questions and also announce when question is answered or new appear on websocket.
Testing