search

podio / podio-dotnet

Podio .NET client
MIT License
19 stars 39 forks source link

Change item id from int to long #59

Open michaelmarziani opened 2 years ago

michaelmarziani commented 2 years ago

The item id for newly created items no longer fits within an int, thus throwing an exception when such items are parsed.

michaelmarziani commented 2 years ago

forgive me if I'm incorrect. I'm looking at this code for the first time. Should public async Task int CloneItem(long itemId, bool silent = false) be Task long?

@Ldatz Thanks for the review and I believe you're correct. I'll update the PR with this fix.

JonTvermose commented 2 years ago

@michaelmarziani There are still a couple of places in ItemService.cs where the itemId is referred as an int

https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L191

https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L228

And for these two I believe the int arrays should be changed to long arrays. https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L475

https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L726

michaelmarziani commented 2 years ago

@JonTvermose Thank you for the review. I agree with all the places you pointed out except the 2nd. The app item id is different than the global item id.

https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L228

I've updated the PR to reflect the 3 fixes you pointed out.

JonTvermose commented 2 years ago

@michaelmarziani

This method probably needs an overload to accept long arrays :) Otherwise I suspect the code wont compile.

https://github.com/podio/podio-dotnet/blob/6187c773b5d19a02595f8cb0ff439707dfe4abc1/Source/Podio%20.NET/Utils/Utilities.cs#L13

Edit: I also see some issues in CommentService.cs where the id can also be an itemId if the type specified is "item" https://github.com/podio/podio-dotnet/blob/6187c773b5d19a02595f8cb0ff439707dfe4abc1/Source/Podio%20.NET/Services/CommentService.cs#L49

https://github.com/podio/podio-dotnet/blob/6187c773b5d19a02595f8cb0ff439707dfe4abc1/Source/Podio%20.NET/Services/CommentService.cs#L79

https://github.com/podio/podio-dotnet/blob/6187c773b5d19a02595f8cb0ff439707dfe4abc1/Source/Podio%20.NET/Services/CommentService.cs#L111

There are probably more places as well, but those are the ones I've encountered so far. I have created a fork of this repository and published a nuget package (https://www.nuget.org/packages/Tvermose.Podio.Async) as our production system has been hit by this error.. Hope things get fixed in this repository eventually so I can revert back and delete the fork :-)

michaelmarziani commented 2 years ago

@JonTvermose Would you be willing to send a PR based on your code? If so I'll cancel mine, as it looks like you've already done this work.

GuidoKuth commented 2 years ago

@JonTvermose Thank you for the review. I agree with all the places you pointed out except the 2nd. The app item id is different than the global item id.

https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L228

I've updated the PR to reflect the 3 fixes you pointed out.

There seems to be one more place that have to be modified. Even if I saw in the code that this is already done it is not in your nuget package. https://github.com/podio/podio-dotnet/blob/d90a1c7c90458417cfe21b1d7130f57014f4eeb4/Source/Podio%20.NET/Services/ItemService.cs#L213

ManuBermudez17 commented 2 years ago

Do you know when they will commit a fix to this issue on podio/podio-dotnet? Will someone publish these updates as packages on nuget?

michaelmarziani commented 2 years ago

@ManuBermudez17 My sense of things is that this is a purely speculative PR, which may some day be merged and re-released, or not. The reason for this is that this library is no longer officially maintained, the repo has been archived, and it's been tagged with "No maintenance intended".

michaelmarziani commented 2 years ago

@JonTvermose Would you be willing to send a PR based on your code? If so I'll cancel mine, as it looks like you've already done this work.

Jon et al,

I ended up integrating all the fixes from your repository. You can see the current diff between what you have and what I have: https://github.com/JonTvermose/podio-dotnet/compare/master..ioSlate:podio-dotnet:master

I believe my code is ready for review now, having integrated all the feedback.

ManuBermudez17 commented 2 years ago

@michaelmarziani I am using the synchronous way, I have tried the asynchronous way and I would have to change several things in the code. Is there any way to get some updated PR of the synchronous part to insert in my project? Because the packages that I see in nuget updated, are asynchronous. And I have tried some asynchronous one and I get several errors regarding the code I have.

michaelmarziani commented 2 years ago

@ManuBermudez17 I wish I could answer that. I noticed myself that when I rebuilt the code, all my sync methods became async and I had to rework them to work with the async code.

What I noticed is that there are 2 directories under Source, one named Podio .NET, the other Podio.Async. All the code is located in the Podio .NET directory, and the code looks like it's built to be async. There must be something in one of the project or solution files (there are many) that tells the code to build without async, but I don't know what it is.

If you have an hints or could point me in the right direction, I'd be happy to try to build a sync version and try to publish it to nuget.

ManuBermudez17 commented 2 years ago

@michaelmarziani I am like you. I have tried to compile from the Podio .NET folder, but as you say, it is asynchronous and several methods that I use fail with errors.

For example:

var filteredItems = podio.ItemService.FilterItems(appId: functionsPodio.appClientesId, filters: filter);
var filteredItemsString =  filteredItems.Filtered.ToString();

In Filtered it tells me that there is no accessible method with that name.

I have in my code:

using PodioAPI;
using PodioAPI.Utils.ItemFields;

Just like looping through a foreach, I did it like this:

foreach (var item in filteredItems.Items)
{
  ...
}

But also in Items, it tells me that there is no accessible method with that name.

And I can't find any manual on how to use the asynchronous way correctly.

ManuBermudez17 commented 2 years ago

@michaelmarziani I have found this branch v1, which I see that the last commits are from 2018 and I do not see the asynchronous folder, which may be the synchronous version. I would have to test it to see if it is valid for the project and I would have to update with the changes proposed by you.

Branch: https://github.com/podio/podio-dotnet/tree/v1/Source

ManuBermudez17 commented 2 years ago

I have tried the branch that I have previously posted, and it works for me with all the code that I had developed synchronously.

I would only have to test the changes made in the files that are named here, to see if everything is correct.

GuidoKuth commented 2 years ago

@JonTvermose I changed the nuget package from podio.async to yours mentioned above but there are some changes missing. Would you be willing to update your nuget package to reflect all latest changes. I think this would help. Thank you for your work.

michaelmarziani commented 2 years ago

I have tried the branch that I have previously posted, and it works for me with all the code that I had developed synchronously.

I would only have to test the changes made in the files that are named here, to see if everything is correct.

@ManuBermudez17 I looked at the v1 branch as well and it looks like up until July 7, 2018 both master and v1 were getting the same updates, so I agree that you could apply the same changes in this PR and use the v1 branch.

About the async code, I didn't have to change much, in case you're thinking of adapting your code to async. I had to change the method signature to return a Task for any methods that called Podio, and then await the Podio method, for example, sync code:

public int FetchItemFromPodio(Podio podio, long itemId)
{
    // Fetch the item
    var item = podio.ItemService.GetItemBasic(itemId);

    // Process the item and return 1 or 0 for success or failure
    // ...
}

Async code:

public async Task<int> FetchItemFromPodio(Podio podio, long itemId)
{
    // Fetch the item
    var item = await podio.ItemService.GetItemBasic(itemId);

    // Process the item and return 1 or 0 for success or failure
    // ...
}

Task is from System.Threading.Tasks.

JonTvermose commented 2 years ago

@JonTvermose I changed the nuget package from podio.async to yours mentioned above but there are some changes missing. Would you be willing to update your nuget package to reflect all latest changes. I think this would help. Thank you for your work.

@GuidoKuth Yes, I have updated the code and created a new package just now.

ManuBermudez17 commented 2 years ago

@michaelmarziani I have created a branch on my github, with only the synchronous part and the changes made to make it work properly. I have tried it in my project and no error is thrown.

The only thing left would be to create the nuget package, but I've never created one.

Repository: https://github.com/ManuBermudez17/podio-dotnet-sync

JonTvermose commented 2 years ago

@michaelmarziani I have created a branch on my github, with only the synchronous part and the changes made to make it work properly. I have tried it in my project and no error is thrown.

The only thing left would be to create the nuget package, but I've never created one.

Repository: https://github.com/ManuBermudez17/podio-dotnet-sync

@ManuBermudez17 If you have visual studio, it's not that difficult to create a nuget package.

https://docs.microsoft.com/en-us/nuget/quickstart/create-and-publish-a-package-using-visual-studio?tabs=netcore-cli

michaelmarziani commented 2 years ago

@AjmalVh @maarand @lowderjh when you have a spare moment, might I ask for a code review and approval if all looks good?

@JonTvermose Thanks for your advice and help.

GuidoKuth commented 2 years ago

@JonTvermose I changed the nuget package from podio.async to yours mentioned above but there are some changes missing. Would you be willing to update your nuget package to reflect all latest changes. I think this would help. Thank you for your work.

@GuidoKuth Yes, I have updated the code and created a new package just now.

Thank you very much @JonTvermose but I think there is still something missing. I get en error in ItmeService.FilterItems cause the returned items still have an int type itemId.

In the meantime I digged a littel bit deeper and I think the Problem is in the ItemField class where the FieldId ist still an int.

Ldatz commented 2 years ago

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

ManuBermudez17 commented 2 years ago

@Ldatz as @michaelmarziani commented, this package has the message that it will not have any more maintenance by Citrix.

https://developers.podio.com/clients/dotnet

michaelmarziani commented 2 years ago

@ManuBermudez17 That's true this package is marked as no maintenance intended, but one of the maintainers, @PavloBasiuk and I exchanged a comment on stackoverflow where they indicated that perhaps if I submitted a PR that it might be merged, but no promises.

In any case, at the end of the day at least we'll all have a nuget package that works, whether this repo or one of our forks. I really appreciate all the people that have joined together to work on sorting this out.

GuidoKuth commented 2 years ago

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

michaelmarziani commented 2 years ago

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

@GuidoKuth Can you post a snippet of the code that generates this error?

Ldatz commented 2 years ago

Where are you getting the error: System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."? After installing the Malte.Podio.Async NuGet package, I then had to change every occurrence of a call with an int item id to a long variable in my application that uses Podio.Async library.

GuidoKuth commented 2 years ago

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

@GuidoKuth Can you post a snippet of the code that generates this error?

Unfortunately not cause the error is thrown outside of my code.

GuidoKuth commented 2 years ago

Where are you getting the error: System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."? After installing the Malte.Podio.Async NuGet package, I then had to change every occurrence of a call with an int item id to a long variable in my application that uses Podio.Async library.

Yes, I have done so. The error seems to be thrown in the library itself cause the error says in first line Thrown by external code.

GuidoKuth commented 2 years ago

If you can't get a good working copy of Podio.Async, maybe you can start with the one called Malte.Podio.Async. I replaced the NuGet package Podio.Async with Malte.Podio.Async 2 weeks ago. I would prefer to install the package maintained by Citrix but, I couldn't wait.

I gave it a try but I stil run into an error System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."

@GuidoKuth Can you post a snippet of the code that generates this error?

Unfortunately not cause the error is thrown outside of my code.

I am very sorry for the confusion but as my project uses man packages and is deeply structured there was another Package Reference for Podio that caused the error. I changed the package reference to the Malte.Podio.Async nuget Package and everything works perfect. Again sorry for confusion and thanks to all for your help.

michaelmarziani commented 2 years ago

Greetings collaborators, @lowderjh thank you for your approval and I wanted to ask if one more reviewer could give a final review and approval here if all looks good.

JonTvermose commented 2 years ago

I have released a new version of my fork where the Comment Id has also been changed to a long. https://www.nuget.org/packages/Tvermose.Podio.Async

Github repo: https://github.com/JonTvermose/podio-dotnet

I have no affiliation with Podio, the fork exists as my production code is dependent on this library.

GuidoKuth commented 2 years ago

I changed all model classes from int to long

Holen Sie sich Outlook für Androidhttps://aka.ms/AAb9ysg

From: Louis Datz @.> Sent: Saturday, July 23, 2022 5:14:37 PM To: podio/podio-dotnet @.> Cc: Guido Kuth @.>; Mention @.> Subject: Re: [podio/podio-dotnet] Change item id from int to long (PR #59)

Where are you getting the error: System.MissingMethodException: "Method not found: 'Int32 PodioAPI.Models.Item.get_ItemId()'."? After installing the Malte.Podio.Async NuGet package, I then had to change every occurrence of a call with an int item id to a long variable in my application that uses Podio.Async library.

— Reply to this email directly, view it on GitHubhttps://github.com/podio/podio-dotnet/pull/59#issuecomment-1193140947, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABRWDSPGEW5NWSAX6MIQCQ3VVQD53ANCNFSM53AMMG7Q. You are receiving this because you were mentioned.Message ID: @.***>