Features: iq::Bind and iq::Roster #4

Open
Ivan-lis wants to merge 3 commits from feature_roster_on_login into main
Member
No description provided.
Ivan-lis added 2 commits 2024-11-17 23:34:48 +00:00
Feature: iq::Roster
Some checks failed
PR Check / on-push-commit-check (push) Failing after 11m10s
7685385559
Author
Member

image

![image](/attachments/c1868912-d42b-4573-9799-5b440ee3b255)
sha512sum requested changes 2024-11-18 07:10:28 +00:00
@ -0,0 +29,4 @@
return {};
}
auto* jid_el = dynamic_cast<const xmlpp::Element*>(jid_node);
Owner

You can create a structure that contains jid as an attribute and use automatic generation of serialization and deserialization

You can create a structure that contains jid as an attribute and use automatic generation of serialization and deserialization
Author
Member

I tried, but roster result should has 'jid' attr, while with Serialization with BareJid I got <NO_NAME username="n" server="s" >

Expected ResultRoster

<iq id='bv1bs71f'
       to='juliet@example.com/chamber'
       type='result'>
    <query xmlns='jabber:iq:roster' ver='ver7'>
      <item jid='nurse@example.com'/>
      <item jid='romeo@example.net'/>
    </query>
  </iq>
I tried, but roster result should has **'jid' attr,** while with Serialization with BareJid I got `<NO_NAME username="n" server="s" >` Expected ResultRoster ```json <iq id='bv1bs71f' to='juliet@example.com/chamber' type='result'> <query xmlns='jabber:iq:roster' ver='ver7'> <item jid='nurse@example.com'/> <item jid='romeo@example.net'/> </query> </iq> ```
Owner

I tried, but roster result should has 'jid' attr, while with Serialization with BareJid I got <NO_NAME username="n" server="s" >

Expected ResultRoster

<iq id='bv1bs71f'
       to='juliet@example.com/chamber'
       type='result'>
    <query xmlns='jabber:iq:roster' ver='ver7'>
      <item jid='nurse@example.com'/>
      <item jid='romeo@example.net'/>
    </query>
  </iq>

See how it's done in SomeStruct5 in tests/serialization.cpp

> I tried, but roster result should has **'jid' attr,** while with Serialization with BareJid I got `<NO_NAME username="n" server="s" >` > > Expected ResultRoster > ```json > <iq id='bv1bs71f' > to='juliet@example.com/chamber' > type='result'> > <query xmlns='jabber:iq:roster' ver='ver7'> > <item jid='nurse@example.com'/> > <item jid='romeo@example.net'/> > </query> > </iq> > ``` > > See how it's done in SomeStruct5 in tests/serialization.cpp
Author
Member

Interesting, looks like last time I did something wrong and got <username="n" server="s"> instead. Will try t use BareJid

Interesting, looks like last time I did something wrong and got <username="n" server="s"> instead. Will try t use BareJid
@ -0,0 +48,4 @@
using IqBind = Iq<Bind>;
inline auto MakeSetBind() {
return SetBind{.id = "1", .payload = Bind{}};
Owner

For what?

For what?
Ivan-lis marked this conversation as resolved
@ -126,2 +146,2 @@
}) //
| std::ranges::to<std::unordered_map<std::string_view, std::string_view>>();
auto params = std::views::split(decoded, ',') | std::views::transform([](auto param) {
return std::string_view{param};
Owner

Why? It was more readable before.

Why? It was more readable before.
Ivan-lis marked this conversation as resolved
@ -72,0 +55,4 @@
std::string username;
std::string server;
constexpr operator FullJid() const {
return FullJid{.username = username, .server = server, .resource = ""};
Owner

Extra copying if there is an rvalue. Better to forward depending on the type with which it is called.

Extra copying if there is an rvalue. Better to forward depending on the type with which it is called.
Ivan-lis marked this conversation as resolved
@ -0,0 +38,4 @@
}
friend constexpr auto operator<<(xmlpp::Element* element, const Roster& roster) {
element->set_attribute("xmlns", Roster::kDefaultNamespace);
std::ranges::for_each(roster.items, [element](const auto& item) {
Owner

Why not range based for?

Why not range based for?
sha512sum marked this conversation as resolved
@ -0,0 +51,4 @@
return {.items = item_nodes | std::views::transform([](const xmlpp::Node* node) {
auto item_element = dynamic_cast<const xmlpp::Element*>(node);
if(!item_element) {
throw std::runtime_error("Can't convert xmlpp::Node to xmlpp::Element");
Owner

You can create a structure that contains jid as an attribute and use automatic generation of serialization and deserialization

You can create a structure that contains jid as an attribute and use automatic generation of serialization and deserialization
@ -0,0 +70,4 @@
using IqRoster = Iq<Roster>;
inline auto MakeGetRoster(const FullJid& jid) {
return GetRoster{.id = "1", .from = ToString(jid), .payload = Roster{}};
Owner

For what?

For what?
Ivan-lis marked this conversation as resolved
@ -297,1 +297,4 @@
template <typename T>
concept LengthCalculatable = requires(const T& obj) {
{ obj.length() } -> std::convertible_to<std::size_t>; // Checks if obj has a length() method returning a type convertible to std::size_t
Owner

A comment that doesn't explain anything and just repeats the code.

A comment that doesn't explain anything and just repeats the code.
sha512sum marked this conversation as resolved
@ -0,0 +10,4 @@
FullJid jid{.username = "test", .server = "server", .resource = "res"}; // NOLINT
auto roster = iq::MakeGetRoster(jid);
roster.payload.items.emplace_back("u1", "s1");
roster.payload.items.emplace_back("u2", "s2");
Owner

Why not just throw all the necessary elements through the vector constructor in Roster constructor?

Why not just throw all the necessary elements through the vector constructor in Roster constructor?
Ivan-lis marked this conversation as resolved
@ -0,0 +22,4 @@
ASSERT_EQ(roster.payload.items.size(), parse_res.payload.items.size());
for(const auto& [idx, expect_el, parsed_el] : std::views::zip(std::views::iota(0), roster.payload.items, parse_res.payload.items)) {
EXPECT_EQ(expect_el, parsed_el) << "Mismatched on idx: " << idx;
// std::cerr << " " << "idx: " << idx << "; expect_el: " << expect_el << "; parsed_el: " << parsed_el << '\n';
Owner

Code left in the comment

Code left in the comment
Ivan-lis marked this conversation as resolved
@ -0,0 +33,4 @@
roster.payload.items.emplace_back("u2", "s2");
roster.payload.items.emplace_back("u3", "s3");
EXPECT_NO_THROW({ std::cerr << "[ ] Roster payload: " << ToString(roster.payload) << '\n'; });
Owner

EXPECT_EQ and test content

EXPECT_EQ and test content
Ivan-lis marked this conversation as resolved
Ivan-lis added 1 commit 2024-11-18 23:06:10 +00:00
Fixed errors and improve roster tests
All checks were successful
PR Check / on-push-commit-check (push) Successful in 12m15s
54b3535ebc
sha512sum requested changes 2024-11-19 13:33:46 +00:00
@ -11,6 +12,8 @@ namespace iq {
template <auto& Name, typename PayloadType>
struct BaseImplWithPayload {
std::string id;
std::optional<std::string> from{};
Owner

Why std::string and not a more limited type like Jid ? Or for get or set use FullJid for from and BareJid in to, and in response or error make FullJid be in to and BareJid in from.

Why std::string and not a more limited type like Jid ? Or for get or set use FullJid for from and BareJid in to, and in response or error make FullJid be in to and BareJid in from.
Ivan-lis force-pushed feature_roster_on_login from 54b3535ebc to d88f14a66d 2024-11-21 22:34:18 +00:00 Compare
Ivan-lis reviewed 2024-11-21 22:36:40 +00:00
@ -75,0 +95,4 @@
co_return;
}
auto UpdateListOfContacts() -> boost::asio::awaitable<void> {
Author
Member

Maybe we can change namings later

Maybe we can change namings later
Ivan-lis force-pushed feature_roster_on_login from d88f14a66d to 1a9b293dee 2024-11-21 22:48:18 +00:00 Compare
Ivan-lis force-pushed feature_roster_on_login from 1a9b293dee to 604b423d1c 2024-11-21 23:15:44 +00:00 Compare
Ivan-lis force-pushed feature_roster_on_login from 604b423d1c to 30a5e69d14 2024-11-21 23:22:15 +00:00 Compare
All checks were successful
PR Check / on-push-commit-check (push) Successful in 11m21s
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature_roster_on_login:feature_roster_on_login
git checkout feature_roster_on_login

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout main
git merge --no-ff feature_roster_on_login
git checkout feature_roster_on_login
git rebase main
git checkout main
git merge --ff-only feature_roster_on_login
git checkout feature_roster_on_login
git rebase main
git checkout main
git merge --no-ff feature_roster_on_login
git checkout main
git merge --squash feature_roster_on_login
git checkout main
git merge --ff-only feature_roster_on_login
git checkout main
git merge feature_roster_on_login
git push origin main
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Larra/larra#4
No description provided.