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

KeyValue: Handle (ignore) conditionals #684

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

yaakov-h
Copy link
Member

As pointed out by @xPaw, we crash when reading tf_english.txt.

I narrowed this down to our handling of conditionals - it's completely broken. We count the conditional as a key, then read the next key as a value, etc. until our keys and values are swapped.

Therefore, an empty value after a conditional will trigger the exception that fails a parse on an empty key.

I don't think this is necessarily the best way to handle them, but at least it doesn't crash 😄

Do not merge (yet), I'm only posting this for discussion. How do we want to handle conditionals, and is this the best way?

I think I can see some implementation flaws in this code (i.e. the green part of the diff) but it's getting late and I need to go to bed so I'm stopping for tonight. I know it's not perfect.

Previous PR and discussion at #232.

@yaakov-h yaakov-h added the bug label May 23, 2019
@yaakov-h yaakov-h added this to the 2.3.0 milestone May 23, 2019
@xPaw
Copy link
Member

xPaw commented May 23, 2019

Worth pointing out that @yaakov-h wrote a different keyvalue parser and it has good test coverage: https://github.com/SteamDatabase/ValveKeyValue

@yaakov-h
Copy link
Member Author

I only did that after stumbling across this and needing a better solution. 😄

@yaakov-h yaakov-h modified the milestones: 2.3.0, 2.4.0 Mar 12, 2020
@yaakov-h yaakov-h modified the milestones: 2.4.0, 2.5.0 Nov 15, 2021
@xPaw xPaw modified the milestones: 2.5.0, 2.next Mar 21, 2023
@xPaw xPaw modified the milestones: Future, Some day (PRs welcome) Aug 2, 2024
@xPaw
Copy link
Member

xPaw commented Aug 29, 2024

I've copied tests from this PR into master: 0c6b643

At this point I'm thinking we should just intentionally not support parsing conditionals and tell people to use the VKV lib if they need to support it.

@yaakov-h
Copy link
Member Author

yaakov-h commented Sep 1, 2024

Is there anything where SteamKit2 parses the KV and makes decisions from that, or where we parse it without letting the consumer access the raw data to parse it themselves?

@xPaw
Copy link
Member

xPaw commented Sep 1, 2024

We only parse appinfo which has no conditionals

EDIT: Can also be used by webapi, but that also comes with no conditionals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants