Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fixed query creation for Option values#57

Open
UnHolds wants to merge 3 commits into
aprimadi:main from
UnHolds:main
Open

Fixed query creation for Option values #57
UnHolds wants to merge 3 commits into
aprimadi:main from
UnHolds:main

Conversation

@UnHolds

@UnHolds UnHolds commented Feb 11, 2024
edited
Loading

Copy link
Copy Markdown

If an option type is used, and it is None, then the tag / field will be excluded from the query, because this is the only working way to transmit null values to the database. (Issue #56 )

This behavior introduces a bug that may lead to an invalid query. The problem arises when a struct contains only option values for the tags and or fields, because then if every option value is None every tag / field will be excluded. This breaks the rule that at least one tag / field must exist.

if tag_writes.len() < 1 {
panic!("You have to specify at least one #[tag] field.")
}
if timestamp_writes.len() != 1 {
panic!("You have to specify at exact one #[timestamp] field.")
}
if fields_writes.len() < 1 {
panic!("You have to specify at least one #[field] field.")
}

In my opinion, this bug is more acceptable than the currently non-functional implementation for options.

I just started out with Rust, so feedback is highly appreciated :)

@UnHolds UnHolds changed the title (削除) Fixed query creation for Option` values #56 (削除ここまで) (追記) Fixed query creation for Option` values (追記ここまで) Feb 11, 2024
@UnHolds UnHolds changed the title (削除) Fixed query creation for Option` values (削除ここまで) (追記) Fixed query creation for Option values (追記ここまで) Feb 11, 2024

Copy link
Copy Markdown
Contributor

per the docs the timestamp and tags are actually both optional so it shouldn't panic if there are no valid tags or a timestamp (and probably support optional timestamps as well)

UnHolds commented Feb 13, 2024

Copy link
Copy Markdown
Author

The timestamp and the tags are now optional. This means it is now also allowed to not use any tags, which addresses #49.
Tests were added to verify the resulting online protocol string.

@poster515 thanks for posting the link to the docs and pointing this out.

But the behavior issue from above is still present when the following struct would be used:

#[derive(WriteDataPoint)]
#[measurement = "something"]
struct Item6 {
 #[influxdb(tag)]
 tag1: Option<String>,
 #[influxdb(field)]
 field1: String,
 #[influxdb(timestamp)]
 time: u64,
}

Here the field1 could be set to None which would lead to an invalid protocol string, because the string needs to include at least one field.
Some feedback on how this should be solved would be nice, or should we just ignore it for now and create an issue?

Copy link
Copy Markdown
Contributor

This seems like a question of "where should the fault occur?". The failure to write the point either happens in this code or at the influx server itself.

I kinda think simply not panicking and letting the write fail at the server is an easy approach but am not a maintainer lol

Copy link
Copy Markdown
Collaborator

Thanks for the contribution and constructive discussion. I hope to give this a closer look later this week. I am leaning towards ensuring that at least 1 field is written and returning an Err if that isn't the case. I'd expect performance overhead to be acceptable. But I'd be happy to add that to this PR myself

UnHolds commented Feb 21, 2024

Copy link
Copy Markdown
Author

Thanks for looking at the PR. It would be nice if you could add this to the PR, because I am currently traveling and won't have much time.

LevBeta commented Jun 9, 2024

Copy link
Copy Markdown

@UnHolds @Boudewijn26 Is this PR abandoned?

Copy link
Copy Markdown
Collaborator

@LevBeta My apologies for my tardiness. I've been quite busy. I'll try to get to this soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /