You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by wido <gi...@git.apache.org> on 2016/01/14 15:27:34 UTC
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
GitHub user wido opened a pull request:
https://github.com/apache/cloudstack/pull/1341
CLOUDSTACK-9238: Increase URL fields to 2048 charachters from 255
255 characters is to small for various URLs like S3 pre-signed URLs.
This causes one or more characters to be chopped of the end of the URL
and this renders them useless.
Internally in the code all URLs are passed as Strings and they are not
sized limited. This was purely in the database.
Other URL fields in the database were already 2048 characters.
This limit was introduced in the 4.1 to 4.2 upgrade when Object storage
like S3 and Swift was introduced in CloudStack for Secondary Storage.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/wido/cloudstack CLOUDSTACK-9238
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1341.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1341
----
commit a171bbc96dc4c44734d6c14265347f9db791a0cb
Author: Wido den Hollander <wi...@widodh.nl>
Date: 2016-01-14T13:06:04Z
CLOUDSTACK-9238: Increase URL fields to 2048 charachters from 255
255 characters is to small for various URLs like S3 pre-signed URLs.
This causes one or more characters to be chopped of the end of the URL
and this renders them useless.
Internally in the code all URLs are passed as Strings and they are not
sized limited. This was purely in the database.
Other URL fields in the database were already 2048 characters.
This limit was introduced in the 4.1 to 4.2 upgrade when Object storage
like S3 and Swift was introduced in CloudStack for Secondary Storage.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-172259203
Thanks @remibergsma !
That is the same reasoning as @kevindierkx and me had while looking into this. We went for 2k for now as it seems to match almost 100% of the use cases.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-172244406
This page has some interesting background:
http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers
It seems URLs can be much longer. The question is whether we need those in CloudStack. For now I'm OK with 2K instead of 256. We can always bump them later when needed.
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by kevindierkx <gi...@git.apache.org>.
Github user kevindierkx commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-171659089
Should resolve problems with template extracts where the generated URL exceeds the 255 character limit.
BTW: 2048 is somewhere in the middle of what is defined as standard (website url length) and what is supported by most browsers.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/cloudstack/pull/1341
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by marcaurele <gi...@git.apache.org>.
Github user marcaurele commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-222071694
I guess a PR is easier, so here you go https://github.com/apache/cloudstack/pull/1567
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by marcaurele <gi...@git.apache.org>.
Github user marcaurele commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-222067277
Hi! Talking with @footplus on the mailing list, I realized that this patch is not correct. It's missing modifications on the VO objects to let the String holds more than 255 characters because in the `GenericDaoBase`, `String` fields are by default of 255 chars, see:
https://github.com/apache/cloudstack/blob/master/framework/db/src/com/cloud/utils/db/GenericDaoBase.java#L1510
This PR should be updated, or should there be a new one to add those changes?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by kevindierkx <gi...@git.apache.org>.
Github user kevindierkx commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-171685523
@borisroman Because VARCHAR can handle 'most common' supported URL lengths perfectly fine.
Most modern browsers have a limit somewhere around 2000 characters.
More information about the matter: http://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-171703995
Like @kevindierkx indeed says. A VARCHAR can be up to 64k characters long. It seems that 2k is the maximum length of a URL used in most systems.
That's why we sticked with the VARCHAR and didn't go for text. A VARCHAR is also a bit more optimal inside MySQL than a TEXT field.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-171967145
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by wido <gi...@git.apache.org>.
Github user wido commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-171657390
I tested this by manually applying all the SQL patches and verifying that all URLs are still in the database like they should be.
Also added a new template and saw that it registered as it should.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9238: Increase URL fields to 2...
Posted by borisroman <gi...@git.apache.org>.
Github user borisroman commented on the pull request:
https://github.com/apache/cloudstack/pull/1341#issuecomment-171681534
@wido @kevindierkx Why not use TEXT? Then we'll never have to worry again :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---