Skip to content

Conversation

@ajolipa
Copy link

@ajolipa ajolipa commented Oct 17, 2025

In this PR

Addresses performant-software/core-data-cloud#523 by checking whether the passed value is already of type Array before applying JSON.parse.

Notes and questions

For maximum flexibility and backwards compatibility I am not assuming here that the value in this case will always be an array; I'm not sure why or at what point something changed so that it's actually being parsed as an array automatically without needing to be parsed, but I figure it can't hurt to just support both cases?

@ajolipa ajolipa requested review from blms and camdendotlol October 17, 2025 15:13
@camdendotlol camdendotlol requested review from dleadbetter and removed request for camdendotlol November 4, 2025 19:33
@dleadbetter
Copy link
Contributor

I'm not sure we want to support multiple ways to store the JSON data. Is it possible that Rails has always been converting the JSON for us and this never worked?

@ajolipa
Copy link
Author

ajolipa commented Nov 4, 2025

I'm not sure we want to support multiple ways to store the JSON data. Is it possible that Rails has always been converting the JSON for us and this never worked?

That is possible! With NBU there was a field ("status" on person records) that was successfully indexed the first few times we ran the Typesense scripts and then at some point the client noticed that newer data wasn't showing up; but in that case I believe the field was changed to multiple in the interim, which I assume is what caused it to stop working. So I don't think I have any evidence that it ever worked -- just without being sure I didn't want to mess things up for older projects! Open to suggestions for the best way to handle it obviously.

@dleadbetter
Copy link
Contributor

Yeah, I think we should assume that the value argument passed to the convert_value method is whatever is pulled directly out of the user_defined JSON hash. I tested this on staging.coredata.cloud, and when we're using a field type of "Select" with "Allow Multiple", the value appears to be an array. If that's not the case for a project/record, I think we have a data typing problem happening somewhere else which we should address separately.

@ajolipa
Copy link
Author

ajolipa commented Nov 6, 2025

Updated to just remove the convert_array part of the logic, per discussion above suggesting we can go ahead and assume that we don't need a special conversion for the case of a select field with allowMultiple.

@dleadbetter dleadbetter added the next release Issues in the next release label Nov 6, 2025
@dleadbetter
Copy link
Contributor

Is there an issue we can connect this to?

@ajolipa
Copy link
Author

ajolipa commented Nov 6, 2025

Is there an issue we can connect this to?

I linked performant-software/core-data-cloud#523 in the description; should I also create an issue in this repo for it?

@dleadbetter
Copy link
Contributor

I linked performant-software/core-data-cloud#523 in the description; should I also create an issue in this repo for it?

Ah, perfect. I missed that. 👍

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

Labels

next release Issues in the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants