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

Save only the relative path of the filename for StaticDirStorage #1990

Merged
merged 23 commits into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions IHP/FileStorage/ControllerFunctions.hs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ storeFileWithOptions fileInfo options = do
|> LBS.writeFile (cs destPath)

let frameworkConfig = ?context.frameworkConfig
pure $ frameworkConfig.baseUrl <> "/" <> objectPath
pure $ objectPath
S3Storage { connectInfo, bucket, baseUrl } -> do
let payload = fileInfo
|> (.fileContent)
Expand Down Expand Up @@ -226,7 +226,13 @@ createTemporaryDownloadUrlFromPathWithExpiredAt validInSeconds objectPath = do
case storage of
StaticDirStorage -> do
let frameworkConfig = ?context.frameworkConfig
let url = frameworkConfig.baseUrl <> "/" <> objectPath
let urlSchemes = ["http://", "https://"]

let url = if any (`isPrefixOf` objectPath) urlSchemes
-- BC, before we saved only the relative path of a file, we saved the full URL. So use it as is.
then "/" <> objectPath
-- We have the relative path, so add the baseUrl.
else frameworkConfig.baseUrl <> "/" <> objectPath

pure TemporaryDownloadUrl { url = cs url, expiredAt = publicUrlExpiredAt }
S3Storage { connectInfo, bucket} -> do
Expand Down Expand Up @@ -409,4 +415,4 @@ storage = ?context.frameworkConfig.appConfig
storagePrefix :: (?context :: ControllerContext) => Text
storagePrefix = case storage of
StaticDirStorage -> "static/"
_ -> ""
_ -> ""
60 changes: 60 additions & 0 deletions Test/FileStorage/ControllerFunctionsSpec.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
module Test.FileStorage.ControllerFunctionsSpec where

import Test.Hspec
import IHP.Prelude
import IHP.FileStorage.ControllerFunctions
import IHP.Controller.Context
import IHP.FrameworkConfig
import IHP.ModelSupport
import IHP.FileStorage.Types
import System.IO.Temp (withSystemTempDirectory)
import qualified Data.ByteString.Lazy as LBS
import qualified Data.Text as Text
import Data.Default (def)
import Data.Time.Clock (getCurrentTime, addUTCTime)
import Network.Wai as Wai (defaultRequest)
import Network.Wai.Parse (FileInfo(..))
import IHP.Controller.RequestContext

tests :: Spec
tests = describe "IHP.FileStorage.ControllerFunctions" $ do
describe "storeFileWithOptions" $ do
it "returns the objectPath without the baseUrl" $ do
withSystemTempDirectory "ihp-test" $ \tempDir -> do
context <- createControllerContext
let ?context = context

let fileInfo = FileInfo
{ fileName = "test.txt"
, fileContentType = "text/plain"
, fileContent = "Hello, world!"
}

result <- storeFile fileInfo "Test.FileStorage.ControllerFunctionsSpec"

result.url `shouldBe` "Test.FileStorage.ControllerFunctionsSpec/test.txt"

describe "createTemporaryDownloadUrlFromPathWithExpiredAt" $ do
it "returns baseUrl concatenated with objectPath when objectPath does not start with http:// or https://" $ do
context <- createControllerContext
let ?context = context
let objectPath = "static/test.txt"
temporaryDownloadUrl <- createTemporaryDownloadUrlFromPath objectPath

temporaryDownloadUrl.url `shouldBe` "http://localhost:8000/static/test.txt"

it "returns '/' concatenated with objectPath when objectPath starts with 'http://' or 'https://'" $ do
context <- createControllerContext
let ?context = context
let objectPath = "https://example.com/static/test.txt"
temporaryDownloadUrl <- createTemporaryDownloadUrlFromPath objectPath

temporaryDownloadUrl.url `shouldBe` "https://example.com/static/test.txt"

createControllerContext = do
let
requestBody = FormBody { params = [], files = [] }
request = Wai.defaultRequest
requestContext = RequestContext { request, respond = error "respond", requestBody, frameworkConfig = error "frameworkConfig" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mpscholten 👋 I suspect this line, with the error, causes the error I get when running the test

image

How can I make it work? (Copied from here)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That GHC error is unrelated to the error call. I just pushed a workaround via 66518de to master (appartently the INLINE triggers the bug in GHC, but chaging that to an optional inline via INLINABLE not triggers the bug). Can you sync with master and then check again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I think now it hits that error

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use IHP.FrameworkConfig.withFrameworkConfig to get a framework config object, like this:

    let withFrameworkConfig = IHP.FrameworkConfig.withFrameworkConfig (pure ())
    describe "createTemporaryDownloadUrlFromPathWithExpiredAt" $ do
        it "returns baseUrl concatenated with objectPath when objectPath does not start with http:// or https://" $ do
            withFrameworkConfig \frameworkConfig -> do
                context <- createControllerContext frameworkConfig
                let ?context = context
                let objectPath = "static/test.txt"
                temporaryDownloadUrl <- createTemporaryDownloadUrlFromPath objectPath

                temporaryDownloadUrl.url `shouldBe` "http://localhost:8000/static/test.txt"

And then use the frameworkConfig passed to createControllerContext instead of the error call

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this I get

  ./IHP/Prelude.hs:92:17: 
  1) IHP.FileStorage.ControllerFunctions.storeFileWithOptions returns the objectPath without the baseUrl
       uncaught exception: ErrorCall
       Could not find FileStorage in config. Did you call initS3Storage from your Config.hs?
       CallStack (from HasCallStack):
         error, called at ./IHP/Prelude.hs:92:17 in main:IHP.Prelude

While trying to figure how to pass a config with the initStaticDirStorage (didn't figure yet), I found aroundAll (withIHPApp WebApplication config) do

Maybe we should use that? (If so, not sure how 😅 )

Copy link
Collaborator Author

@amitaibu amitaibu Oct 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to init the storage here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests passing locally ✌️

let ?requestContext = requestContext
newControllerContext
2 changes: 2 additions & 0 deletions Test/Main.hs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import qualified Test.RouterSupportSpec
import qualified Test.ViewSupportSpec
import qualified Test.ServerSideComponent.HtmlParserSpec
import qualified Test.ServerSideComponent.HtmlDiffSpec
import qualified Test.FileStorage.ControllerFunctionsSpec
import qualified Test.FileStorage.MimeTypesSpec
import qualified Test.DataSync.DynamicQueryCompiler
import qualified Test.IDE.CodeGeneration.MigrationGenerator
Expand Down Expand Up @@ -78,6 +79,7 @@ main = hspec do
Test.ViewSupportSpec.tests
Test.ServerSideComponent.HtmlParserSpec.tests
Test.ServerSideComponent.HtmlDiffSpec.tests
Test.FileStorage.ControllerFunctionsSpec.tests
Test.FileStorage.MimeTypesSpec.tests
Test.DataSync.DynamicQueryCompiler.tests
Test.IDE.SchemaDesigner.SchemaOperationsSpec.tests
Expand Down
Loading