umbraco / Umbraco.Cms.Integrations

MIT License
31 stars 20 forks source link

fix path property from list<string> to string after change on IRecordbuilder #184

Closed gardarthorsteins closed 2 months ago

gardarthorsteins commented 2 months ago

After changing the IRecordbuilder to inherit from IPublishedContent the index could not be serialized.

There was a difference in how Path property is set up. Changing it from List<string to string fixed the issues.

Tested full rebuild of the index and also publish a single node.

acoumb commented 2 months ago

Hi @gardarthorsteins ,

The update to change the Path from string to List<string> was introduced with this PR, to allow filtering indices by ids in the path.

Strangely how your tests with version 2.1.2 of Algolia, containing this update, did not come across this issue.

I will hold merging this to not impact the persons who submitted the previous PR, and do some tests on my side to see what is happening.

In the meantime, could you share with me a sample structure of a content type that is causing the issue?

Thank you, Adrian

gardarthorsteins commented 2 months ago

Hi @acoumb

I did my testing on a clean installation on Umbraco 13.2.2 with two types of simple document types. Frontpage and Subpage with 1 Content text field on each.

I tested the project with version 2.0 up to 2.1.2 and all failed with the same error I got before.

I then put in the fix I added and everything worked with out issues, I also tested the 2.1.3 package later and it worked.

But when testing the package on a bigger client site I got the Path error in this thread so I thought I made a mistake in my testing or I needed a more complex testing. With this fix here everything worked correctly and the path error was gone.

I have been testing it better now and it seems to be an issue when trying to update a record that already exist in the index, but adding to a fresh index had no issues and that's probably why I did not get this error in my clean project.

If I rebuild the whole index manually the issue goes away so the solution could be that users will have to rebuild the index or we could keep this fix in and add another property on the record model that could split the Path property and return an array of strings but it seems not an elegant solution.

public string Path { get; set; } public string[] PathArray => Path.Split(',');

acoumb commented 2 months ago

Hi @gardarthorsteins ,

Thanks for sharing the details with me. I've tried various setups and configurations, local and with Umbraco Cloud trying to replicate what you've encountered, and no matter how hard I tried to crack it, I just could not get any exceptions.

My setups were with Umbraco 13 and a Clean starter kit, eg: image

I think that at this stage we should probably try figuring out why is this happening on the client's site, in a specific context, rather than patching things up and then experiencing something else.

Maybe you could share with me a content type configuration, property details, what type of project is it (Umbraco Cloud or on premise). I could share with you my test cloud environment that you can configure if this is an option.

Let me know what option suits you so we can get to the bottom of this.

Regards, Adrian

gardarthorsteins commented 2 months ago

I agree, we should wait with this patch.

I will try to replicate the issue again on client sites and give you my findings soon with more data.

acoumb commented 2 months ago

Closing this as it is handled with PR #185