r28087 - What's Changed [*]Create JSON API for relay scripts. by ]@phulin] in
- Thread starter Github Bot
- Start date
@fronobulax raised some points in the pull request that I feel warrant more discussion / visibility.
@ikzann @Ryo_Sangnoir
The obvious place where faster JSON might be relevant is in parsing api.php, but it's also not clear to me that JSON handling is a bottleneck at this time. It's also not clear why we'd use this over, say, simdjson-java which benchmarks favorably against fastjson2. (Okay, one reason is that simdjson-java requires Java 18. Although... we've been talking about upgrading Mafia to require Java 21, which is the latest LTS.)
@ikzann @Ryo_Sangnoir
phulin said:
fronobulax said:
The obvious place where faster JSON might be relevant is in parsing api.php, but it's also not clear to me that JSON handling is a bottleneck at this time. It's also not clear why we'd use this over, say, simdjson-java which benchmarks favorably against fastjson2. (Okay, one reason is that simdjson-java requires Java 18. Although... we've been talking about upgrading Mafia to require Java 21, which is the latest LTS.)
fastjson2 has me worried because it's hundreds of thousands of lines of Java across thousands of files over thousands of commits, and it has a bunch of features we don't need (read: broad attack surface). Based on the "projects using us" list, I also don't see much adoption by other large organizations (there's a handful of Apache projects, mostly opensourced by AliBaba itself, although I saw one or two that came from XiaoMi).
If there really is a compelling use case, then I'm open to discussing it. But I feel like this just piggybacked on another change, and I'd rather we didn't add new external dependencies lightly.
If there really is a compelling use case, then I'm open to discussing it. But I feel like this just piggybacked on another change, and I'd rather we didn't add new external dependencies lightly.
Thank you. I never seem to know where to start discussions and am often not aware of socialization type discussions that occur "elsewhere".
I chose not to mention it on GitHub but when I was developing and deploying code for selected US Government agencies I would not have been allowed to use fastjson2. Basically a known vulnerability in an earlier version combined with unverified allegations that a somewhat antagonistic, non-US entity was heavily involved in the development would have resulted in a decision that is was safer to forbid the use of fastjson2 than to spend the effort to accredit it and "prove" it was safe. I understand that there are no national security implications of KoLmafia but the previous vulnerability did allow a malicious user to run an arbitrary operating system command. So I expect we KoLmafia devs would have taken steps to address that vulnerability and would do so in the future if fastjson2 introduces something similar.
Among the many rules of thumb that guided my development decisions was the one that said performance optimization for its own sake needs to be supported by benchmarks. I still would like to understand who is going to see better performance and under what circumstances. I also acknowledge that just because "I have a bad feeling about this" does not mean my "feeling" will come true.
I chose not to mention it on GitHub but when I was developing and deploying code for selected US Government agencies I would not have been allowed to use fastjson2. Basically a known vulnerability in an earlier version combined with unverified allegations that a somewhat antagonistic, non-US entity was heavily involved in the development would have resulted in a decision that is was safer to forbid the use of fastjson2 than to spend the effort to accredit it and "prove" it was safe. I understand that there are no national security implications of KoLmafia but the previous vulnerability did allow a malicious user to run an arbitrary operating system command. So I expect we KoLmafia devs would have taken steps to address that vulnerability and would do so in the future if fastjson2 introduces something similar.
Among the many rules of thumb that guided my development decisions was the one that said performance optimization for its own sake needs to be supported by benchmarks. I still would like to understand who is going to see better performance and under what circumstances. I also acknowledge that just because "I have a bad feeling about this" does not mean my "feeling" will come true.
I was not going to mention the China angle, but since you mention it:
I am not saying by any means that the CCP has backdoored this project (or has intentions to do so), but they certainly would have the means if they were so inclined.
I am also not saying that the only security vulnerabilities are from state-sponsored actors, or that our threat model should take them into consideration. But taking up unnecessary dependencies on feature-heavy projects is how we get problems like Log4Shell (which, let's be clear, was disclosed by a member of Alibaba's security org!).
I don't think there's any verification needed to state that Alibaba is a Chinese-owned company headquartered in China, and the vast majority of their employees are also based in China.
I am not saying by any means that the CCP has backdoored this project (or has intentions to do so), but they certainly would have the means if they were so inclined.
I am also not saying that the only security vulnerabilities are from state-sponsored actors, or that our threat model should take them into consideration. But taking up unnecessary dependencies on feature-heavy projects is how we get problems like Log4Shell (which, let's be clear, was disclosed by a member of Alibaba's security org!).
Still waiting for a response.
I see that textui/Datatypes.java has been modified.
Can you provide some documentation about how scripters will be able to use this?
Core changes to KolMafia need to be announced - and documented - here.
Whether or not they were discussed on another forum is irrelevant.
This is the “official” forum for announcing KoLmafia changes.
Make an announcement, please.
Thanks!
I see that textui/Datatypes.java has been modified.
Can you provide some documentation about how scripters will be able to use this?
Core changes to KolMafia need to be announced - and documented - here.
Whether or not they were discussed on another forum is irrelevant.
This is the “official” forum for announcing KoLmafia changes.
Make an announcement, please.
Thanks!
Had to find this myself, but I personally don't think this will be documented as such, with the recommended approach being to use
github.com
GitHub - loathers/tome: Tome is a library written to support relay scripts for KoLMafia written in Typescript with React.
Tome is a library written to support relay scripts for KoLMafia written in Typescript with React. - loathers/tome
github.com
My new policy:
For all new feature request PRs, if there is not a kolmafia.us thread explaining exactly what the desired feature is, how it will be implemented, and providing detailed documentation of the new API (RuntimeLibrary, textui/command/*, whatever), as well as complete tests, I will make a blocking review on the PR until such a thread is created and the expected documentation is provided.
Don't do this. I still don't have a clue who this PR will benefit, and how.
I have relay (and non-relay) scripts which use JSON.
Heck, I have built-in KoLmafia support for the nemesis volcano maze in the relay browser which interacts with KoL using JSON.
Maybe that could be a relay script - and maybe this PR could have provided what I needed, since my Java + JS implementation is, essentially, a built-in relay script - but I don't have the tiniest clue whether this PR would be useful to me.
I guarantee that if I wrote a relay script for that, it would be in ASH, rather than JS (which is unreadable) or typescript (which requires its own development environment).
Still waiting for documentation.
For all new feature request PRs, if there is not a kolmafia.us thread explaining exactly what the desired feature is, how it will be implemented, and providing detailed documentation of the new API (RuntimeLibrary, textui/command/*, whatever), as well as complete tests, I will make a blocking review on the PR until such a thread is created and the expected documentation is provided.
Don't do this. I still don't have a clue who this PR will benefit, and how.
I have relay (and non-relay) scripts which use JSON.
Heck, I have built-in KoLmafia support for the nemesis volcano maze in the relay browser which interacts with KoL using JSON.
Maybe that could be a relay script - and maybe this PR could have provided what I needed, since my Java + JS implementation is, essentially, a built-in relay script - but I don't have the tiniest clue whether this PR would be useful to me.
I guarantee that if I wrote a relay script for that, it would be in ASH, rather than JS (which is unreadable) or typescript (which requires its own development environment).
Still waiting for documentation.
ikzann
Member
Hi folks - didn't see this thread until now, apologies. For some reason, the pinging feature that is supposed to go to my email is not working. I will figure that out.
The basic purpose of this change was to provide an efficient way for relay scripts to make large and/or frequent requests for data from inside Mafia via the RuntimeLibrary API. I apologize if I didn't go about submitting it the right way—I've contributed to mafia a number of times over the years, but I wouldn't say I've been a frequent or deep contributor. I thought it was OK to submit with complete tests and documentation in the code. I'm hearing that's not enough.
I'm happy to provide any other documentation for this that folks need. There is some in the code, which I can augment if needed. It sounds like the team would like a page on the wiki and what else?
If we want to use a different library instead of fastjson2, we certainly could. It would not be hard to convert over. I picked it because it has the best score on a benchmark repo I found, and also seems to be pretty feature-complete.
My current use case for this is, as irrat posted, a library called "tome" which enables seamless use of kolmafia APIs in relay scripts. I'm working on a TourGuide replacement that uses this API to create an easier development environment for new content. My understanding is that soolar is also looking into whether CHIT could use it as well.
The basic purpose of this change was to provide an efficient way for relay scripts to make large and/or frequent requests for data from inside Mafia via the RuntimeLibrary API. I apologize if I didn't go about submitting it the right way—I've contributed to mafia a number of times over the years, but I wouldn't say I've been a frequent or deep contributor. I thought it was OK to submit with complete tests and documentation in the code. I'm hearing that's not enough.
I'm happy to provide any other documentation for this that folks need. There is some in the code, which I can augment if needed. It sounds like the team would like a page on the wiki and what else?
If we want to use a different library instead of fastjson2, we certainly could. It would not be hard to convert over. I picked it because it has the best score on a benchmark repo I found, and also seems to be pretty feature-complete.
My current use case for this is, as irrat posted, a library called "tome" which enables seamless use of kolmafia APIs in relay scripts. I'm working on a TourGuide replacement that uses this API to create an easier development environment for new content. My understanding is that soolar is also looking into whether CHIT could use it as well.
Hi,
I also didn't receive an alert for this thread. I don't get alerts for "GitHub changes" more generally: perhaps I disabled them, or perhaps they're disabled for everyone.
I asked bean to add a docstring for the json API in the code: I figured that was enough, given the wiki doesn't have pages for "submitCommand", "redirectedCommand", "specialCommand" and so on, and you have to figure out how those work by looking in the codebase, and they don't have any documentation. I've requested he add a page to the wiki -- currently "jsonApi" but if folk want it somewhere else that's fine too. I can't find any pages for the existing commands so I assume there's no summary page either.
As I understand it, the use-case for this API is for people who want to develop React scripts where, instead of refreshing the page, the script makes a call to an API using JavaScript and updates itself. Ezandora did this in Guide by making her own API ASH script that behaved differently depending on whether you accessed it by POST or GET. This was deemed quite difficult to understand by those who forked the script afterwards. Normal ASH scripts can just make another server call instead of needing to do this: benefits include maintaining the scroll location, which is only really relevant if your page is very long (e.g., it is a Guide-like).
These API scripts can be written as relay scripts, but it's faster to have Java support.
On the JSON library change: bean has done the majority of the performance PRs, and when I checked my own inventory (very large) it seemed faster, so I figured it was a good change. simd-json looks great, and I'm happy to make an announcement that we're eventually going to require Java 21 to start a timeline for getting ready to put it in.
I also didn't receive an alert for this thread. I don't get alerts for "GitHub changes" more generally: perhaps I disabled them, or perhaps they're disabled for everyone.
I asked bean to add a docstring for the json API in the code: I figured that was enough, given the wiki doesn't have pages for "submitCommand", "redirectedCommand", "specialCommand" and so on, and you have to figure out how those work by looking in the codebase, and they don't have any documentation. I've requested he add a page to the wiki -- currently "jsonApi" but if folk want it somewhere else that's fine too. I can't find any pages for the existing commands so I assume there's no summary page either.
As I understand it, the use-case for this API is for people who want to develop React scripts where, instead of refreshing the page, the script makes a call to an API using JavaScript and updates itself. Ezandora did this in Guide by making her own API ASH script that behaved differently depending on whether you accessed it by POST or GET. This was deemed quite difficult to understand by those who forked the script afterwards. Normal ASH scripts can just make another server call instead of needing to do this: benefits include maintaining the scroll location, which is only really relevant if your page is very long (e.g., it is a Guide-like).
These API scripts can be written as relay scripts, but it's faster to have Java support.
On the JSON library change: bean has done the majority of the performance PRs, and when I checked my own inventory (very large) it seemed faster, so I figured it was a good change. simd-json looks great, and I'm happy to make an announcement that we're eventually going to require Java 21 to start a timeline for getting ready to put it in.
I will defer to @Veracity here, but I'll try to address some of the comments / questions.
Evidently, my expectation was wrong, and I shouldn't have assumed the worst.
I at least have some questions about the example provided, even if I don't immediately plan on using the feature.
I am still unsure where to find API documentation for fastjson2. Should I be relying on javadoc.io? What about a migration guide from org.json or other libraries indicating common pitfalls (such as the differing behavior in JSONArray's constructor)?
I expected both of you would have received an alert when I mentioned both of you directly in #3. https://kolmafia.us/account/preferences has an option for receiving notifications (via the bell icon in the top right) for mentions, but I don't see an obvious way to turn that into an email. I didn't want to believe that both of you were willfully ignoring the thread, but that was how it looked (from my perspective, and possibly that of several others) when the migration then completed the next day without any broader discussion about fronobulax's concerns.
Evidently, my expectation was wrong, and I shouldn't have assumed the worst.
It would be good to have a page on the wiki briefly describing the feature and perhaps providing a few simple usage examples, since we don't expect most users to interact deeply with the source code. There are some of these details in the initial PR description itself, but that requires even more steps to find once you've discovered the feature in the source code.
I at least have some questions about the example provided, even if I don't immediately plan on using the feature.
- Can I specify other means of identifying an object, e.g. itemId?
- What about objects that have non-unique names (e.g. "rock" or "Staff of Ed")? Does pilcrow syntax work for those? Do we use Mafia's name resolution in general?
- Is the value passed into objectType case-sensitive? If so, why is "Item" capitalized?
- What does the response look like?
My understanding is that Veracity was initially frustrated with the lack of communication here, but the last straw was the introduction of a couple of bugs as a result of the migration (which only really modified existing tests). Yes, it'd be great if our test coverage were sufficient to catch issues with refactors like this, but we're nowhere near there yet.
I am still unsure where to find API documentation for fastjson2. Should I be relying on javadoc.io? What about a migration guide from org.json or other libraries indicating common pitfalls (such as the differing behavior in JSONArray's constructor)?
I ask that we resolve to have a discussion before implementing "policies" in the development of the project, especially ones gatekeeping contribution. I'm not strictly against this one in particular but I am definitely not convinced. I certainly would not follow it for my own feature PRs, so as it stands would rule me out of future contribution to the project.
I did not (or I did, but it cleared from my alerts before I noticed it was there). I do have the preference enabled for mentions; I don't know if it works at all. I do get alerts for when people like my posts or quote them (as you have done in this thread).
I feel like most of these questions are answered in the JavaDoc for handleJsonApi, which will soon (I expect) be duplicated on the wiki.
- you can pass items by id: "Enumerated types (Item, Familiar, Modifier, etc.) can be passed in with either of the following placeholder formats: { objectType: EnumeratedTypeName, identifierString: string } | { objectType: EnumeratedTypeName, identifierNumber: number }"
- the value passed to objectType is case-sensitive. It's capitalised because the names of types are the JavaScript types: this isn't explicit but the docs do have "Names of functions are in JS style (camelCase)."
- on response, we have "The response will have type { error: string } or { properties?: string[], functions?: unknown[] }, with properties and functions each present if they were present on the request object."
"Enumerated types will be returned as a POJO with all the fields of a JS enumerated type, except any second-level referenced objects will be left as placeholders. Objects will also have the placeholder fields (objectType, identifierString, identifierNumber if applicable) set." - non-unique names: no idea, undefined behaviour, could be clarified with a test.
Personally I use "go to definition" from an IDE with the library installed, same as I do for most other libraries. javadoc.io is also fine. I can't find a proper site but then I can't find one for org.json either.
Regarding user visible features, my own practice is:
1) Often - but not always - write a Feature Request on this forum describing what I intend
If so, read comments and suggestions and engage in discussion.
2) Implement it and open a PR.
In the PR, the initial comment documents the user visible details. Verbosely and in detail.
(I think I tend to be a lot more verbose in my PR comments than some people. You don't have to follow me in that.
3) After the PR is merged, update and close the original Feature Request (if any).
Otherwise, announce and describe the new feature, somewhere.
Perhaps by opening a Feature Request, putting in doc, and marking it Implemented.
Or perhaps even an Announcement, if it's something big, like the Connection Options login retry settings.
Note that I, personally don't update the Wiki - even for new ASH features.
That would be a personal character flaw, I guess.
For non-user-visible features - such as when I refactored CoinmasterData to allow fluid construction - I will not provide a Feature Request on this forum. The doc is in the PR - and in comments in the code. For that particular example, a reviewer requested JavaDoc. I complied, although I don't actually know how to see it, other than as a structured comment. I don't use an IDE which will display it, for example.
This particular feature is user-visible. Its audience is script writers - not KoLmafia devs.
Perhaps the audience is actually the author of a single specific script (library?), who happened to implement the feature?
In any case, I don't think script writers should be required to look at KoLmafia source code and read JavaDoc.
The thing which got me, in this case, was that I noticed this mysterious PR being developed that provides an API for script authors, and then it was merged without any announcement and, well, script authors were supposed to just magically know about it?
I m happy to have a discussion about this. We could start from here...
(For Bug Fixes, if it's something I noticed myself, I sometimes - but not always - make a Bug Report before fixing. If it's a bug in something used by scripters, and fixing it changes script-visible behavior, I'd like to have a Bug Report to at least document the issue.)
I expect script authors are reading the commit history / release notes for new features (new item support, new quest support, new skills, new commands, new functions). I only post major or breaking changes in Announcements. I don't think this is a major change. I wouldn't have thought to post an announcement for it.
We don't make many announcements as is -- apparently none in the last year
The new commands are also not explicit, but folded into the "new content" threads. Somebody makes a post asking for photo booth support, and implicit in that is they either want an ASH function or a command, and I tend to do commands. And sometimes I post a revision in the threads but most of the time I just leave it -- commands are discoverable through "help" and functions through "ashref".
I read the PR comments for this commit.
This PR creates the endpoint that that library will use.And what is the "endpoint"?
Requests to the endpoint take two parameters, body and pwd. pwd needs to match the password hash. body is a JSON object like the following:OK. I can use
to_json() to turn objects into JSON representation.I can pass arguments to the "endpoint".
What, exactly, does that mean?
How do I invoke this "endpoint"?
It is invoked by relay scripts. Is it a function?
Diving into the code, it looks like the "endpoint" is named "jsonApi".
You invoke it by running a RelayRequest where the URL is "KoLmafia/jsonApi?pwd=PWD&body=JSON".
RelayRequest will look up properties and functions, as specified in the BODY argument, and put a JSON string into the responseText.
So, apparently, the relay script gets a RelayRequest, sets the URL and runs it - via
visit_url() - and the responseText is returned as a buffer, as always. Presumably the relay script can use from_json() to decode that result.Except that function doesn't exist.
Oh. It looks like some code is added to textui/javascript.
Technically, any "relay script" could call this "endpoint" but ASH scripts would need, as always, their own JSON-to-object conversion functions.
Whatever.
But, my observation is, look how much I had to dive into the code in order to figure out to invoke this "endpoint".
Is that reasonable for something which is intended for "relay script authors"?
Or is this really only for the use of the author of the PR?
I and a few others are working on a library that allows relay scripts to seamlessly call into KoLmafia functions, largely focused on the Guide replacement YORICK. This PR creates the endpoint that that library will use.Ah. Just so.
ikzann
Member
Maybe it was a little inaccurate to say it will be used by "relay scripts." It can be used by any client-side code running in the relay browser. Whether client-side code is generated by an ASH or JS mafia relay script, the code that runs client-side is all javascript by necessity, as that's the only language (ish) browsers can run. Most ASH scripts I'm aware of do have client-side JS code that runs. Is that the source of the confusion?
Again, I'm happy to add more documentation, fix any resulting bugs, etc. I just haven't had time to do that since discovering this thread yesterday. This PR was not for any nefarious purpose and, while I did write it with one use case in mind, I aimed to write it in a broad enough way to be useful for many purposes. I am also happy to answer questions. I will say that from my end it feels like this discussion is contentious when it doesn't need to be, and I honestly find it a little discouraging in terms of making future contributions.
Again, I'm happy to add more documentation, fix any resulting bugs, etc. I just haven't had time to do that since discovering this thread yesterday. This PR was not for any nefarious purpose and, while I did write it with one use case in mind, I aimed to write it in a broad enough way to be useful for many purposes. I am also happy to answer questions. I will say that from my end it feels like this discussion is contentious when it doesn't need to be, and I honestly find it a little discouraging in terms of making future contributions.