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

Character restrictions for item ID #30

Closed
expertsender-marcinsynak opened this issue Oct 4, 2018 · 4 comments
Closed

Character restrictions for item ID #30

expertsender-marcinsynak opened this issue Oct 4, 2018 · 4 comments

Comments

@expertsender-marcinsynak
Copy link
Contributor

We implement product recommendations solutions based on this project on many websites for our customers.

We've encountered websites which use different formats as their item IDs. Some use simple numbers but these strings could also be valid item IDs in some systems:

J4Z18-JSOM100-27M+31S+36S
[S4Z17-TSDLF701] TSDLF701 - kobalt
g.2017.12.x.xmas-gift-1
head&shoulders_2014_M7_G1
Charmine Rose_2015_M10_G2

It's not up to us to decide whether these formats are good or not - it's just how the real world looks like.

Unfortunately, it is impossible to use these strings as item identifiers in Product-Recommendations, because it imposes character restrictions on item ID in catalog file. Allowed characters are letters, numbers. dash and underscore. There is a piece of code in CatalogFileParser.cs that makes the check:

// check for illegal characters in the item id
if (!itemId.All(UsageEventsFilesParser.IsAlphanumericDashOrUnderscoreCharacter))
{
    parsingError = ParsingErrorReason.IllegalCharactersInItemId;
    return null;
}

The same check is performed when parsing usage events file.

My question is: is it really necessary to be so restrictive when it comes to product ID? Why not just allow any string? We've seen all kinds of special characters, spaces and even national characters in IDs.

Right now we would need to create some kind of ID translator and meticulously convert IDs back and forth every time we feed or retrieve data from Product-Recommendations.

@natinimni
Copy link
Contributor

Hi @marcin-synak ,
I think the system should work with any string, albeit one might need to escape it if some item id is passed in the URL (like in some API).
You could defiantly modify the code locally to do that, build and update your existing service\create a new service, see Deploying New Code to the Deployed Solution

@expertsender-marcinsynak
Copy link
Contributor Author

Hello @natinimni,
Thanks for the answer. We are familiar with deploying procedure. We've updated our solution at least twice if I remember correctly.

Of course, we could modify the code ourselves and remove the offending lines ;) We were just unsure if there were any internal reasons preventing us from doing it (it's hard to say without analyzing the whole source). URL encoding etc is basic stuff. I also assume any valid CSV file (with quotes, escape characters etc) would work.

But, to be honest, we're not thrilled with the idea of maintaining our own version of the code.
Is there a chance such change could hit the master branch? If it's not harmful?

@natinimni
Copy link
Contributor

Hi @marcin-synak ,

looking back a the source, I don't think it should be a problem. Item ids are indexed into integers before being sent to the underlying engine so I cannot see any blocker to remove char restriction from item ids.

Feel free to contribute this and any other useful changes you maintain locally back to the master branch, I can approve your pull request. I hope that's alright.
Thanks!

expertsender-marcinsynak added a commit to expertsender-marcinsynak/Product-Recommendations that referenced this issue Oct 10, 2018
@expertsender-marcinsynak
Copy link
Contributor Author

Thanks Nati, I've created a pull request with my changes.

This is the first time I'm using Git and Github, I hope I did everything right :)

natinimni pushed a commit that referenced this issue Oct 12, 2018
* Removed character restrictions on Item IDs (Issue #30)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants